You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "gerlowskija (via GitHub)" <gi...@apache.org> on 2023/02/14 21:14:08 UTC
[GitHub] [solr] gerlowskija opened a new pull request, #1358: SOLR-16488: Convert ZookeeperReadAPI to JAX-RS
gerlowskija opened a new pull request, #1358:
URL: https://github.com/apache/solr/pull/1358
https://issues.apache.org/jira/browse/SOLR-16488
# Description
Solr is slowly moving towards JAX-RS for its v2 API definitions. This commit switches ZookeeperReadAPI over to use the JAX-RS framework.
# Solution
The migrations was mostly seamless, though the change did require the creation of a new Jersey `MessageBodyWriter` to expose Solr's existing `RawResponseWriter`.
# Tests
No tests (yet).
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on pull request #1358: SOLR-16488: Convert ZookeeperReadAPI to JAX-RS
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1358:
URL: https://github.com/apache/solr/pull/1358#issuecomment-1430406466
FYI @joshgog - I think this PR should serve as a successor to #1144, in light of my initial mistake overlooking these v2 APIs.
This PR leaves the endpoints themselves unchanged (in terms of HTTP verb, path, etc.). But I do think, while we're touching these APIs and since we're going through this process for all of our v2 APIs, that we should rethink what these APIs look like.
(Most of the API-appearance changes have already been proposed, reviewed, etc. in bulk, but obviously these APIs missed that since I didn't find them until just recently.)
I'll post suggestions for those API changes shortly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on pull request #1358: SOLR-16488: Convert ZookeeperReadAPI to JAX-RS
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #1358:
URL: https://github.com/apache/solr/pull/1358#issuecomment-1442136198
I've updated the API paths based on the discussion on SOLR-16488, and added some additional tests and some OpenAPI annotations.
Will run tests on this and schedule a commit sometime soon unless anyone else has feedback?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija merged pull request #1358: SOLR-16488: Convert ZookeeperReadAPI to JAX-RS
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija merged PR #1358:
URL: https://github.com/apache/solr/pull/1358
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a diff in pull request #1358: SOLR-16488: Convert ZookeeperReadAPI to JAX-RS
Posted by "sonatype-lift[bot] (via GitHub)" <gi...@apache.org>.
sonatype-lift[bot] commented on code in PR #1358:
URL: https://github.com/apache/solr/pull/1358#discussion_r1116051093
##########
solr/core/src/java/org/apache/solr/response/RawResponseWriter.java:
##########
@@ -56,9 +66,20 @@ public void init(NamedList<?> n) {
}
}
- // Even if this is null, it should be ok
protected QueryResponseWriter getBaseWriter(SolrQueryRequest request) {
- return request.getCore().getQueryResponseWriter(_baseWriter);
+ if (request.getCore() != null) {
+ return request.getCore().getQueryResponseWriter(_baseWriter);
+ }
+
+ // Requests to a specific core already have writers, but we still need a 'default writer' for
+ // non-core
+ // (i.e. container-level) APIs
+ synchronized (this) {
+ if (defaultWriter == null) {
+ defaultWriter = new JSONResponseWriter();
+ }
+ }
+ return defaultWriter;
Review Comment:
<picture><img alt="5% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/5/display.svg"></picture>
<b>*THREAD_SAFETY_VIOLATION:</b>* Read/Write race. Non-private method `RawResponseWriter.getBaseWriter(...)` reads without synchronization from `this.defaultWriter`. Potentially races with write in method `RawResponseWriter.getBaseWriter(...)`.
Reporting because this access may occur on a background thread.
---
<details><summary>âšī¸ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[â[đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=405095286&lift_comment_rating=1)â] - [â[đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=405095286&lift_comment_rating=2)â] - [â[đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=405095286&lift_comment_rating=3)â] - [â[đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=405095286&lift_comment_rating=4)â] - [â[đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=405095286&lift_comment_rating=5)â]
##########
solr/core/src/java/org/apache/solr/response/RawResponseWriter.java:
##########
@@ -56,9 +66,20 @@ public void init(NamedList<?> n) {
}
}
- // Even if this is null, it should be ok
protected QueryResponseWriter getBaseWriter(SolrQueryRequest request) {
- return request.getCore().getQueryResponseWriter(_baseWriter);
+ if (request.getCore() != null) {
+ return request.getCore().getQueryResponseWriter(_baseWriter);
Review Comment:
<picture><img alt="5% of developers fix this issue" src="https://lift.sonatype.com/api/commentimage/fixrate/5/display.svg"></picture>
<b>*THREAD_SAFETY_VIOLATION:</b>* Read/Write race. Non-private method `RawResponseWriter.getBaseWriter(...)` reads without synchronization from `this._baseWriter`. Potentially races with write in method `RawResponseWriter.init(...)`.
Reporting because this access may occur on a background thread.
---
<details><summary>âšī¸ Expand to see all <b>@sonatype-lift</b> commands</summary>
You can reply with the following commands. For example, reply with ***@sonatype-lift ignoreall*** to leave out all findings.
| **Command** | **Usage** |
| ------------- | ------------- |
| `@sonatype-lift ignore` | Leave out the above finding from this PR |
| `@sonatype-lift ignoreall` | Leave out all the existing findings from this PR |
| `@sonatype-lift exclude <file\|issue\|path\|tool>` | Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file |
**Note:** When talking to LiftBot, you need to **refresh** the page to see its response.
<sub>[Click here](https://github.com/apps/sonatype-lift/installations/new) to add LiftBot to another repo.</sub></details>
---
<b>Help us improve LIFT! (<i>Sonatype LiftBot external survey</i>)</b>
Was this a good recommendation for you? <sub><small>Answering this survey will not impact your Lift settings.</small></sub>
[â[đ Not relevant](https://www.sonatype.com/lift-comment-rating?comment=405095747&lift_comment_rating=1)â] - [â[đ Won't fix](https://www.sonatype.com/lift-comment-rating?comment=405095747&lift_comment_rating=2)â] - [â[đ Not critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=405095747&lift_comment_rating=3)â] - [â[đ Critical, will fix](https://www.sonatype.com/lift-comment-rating?comment=405095747&lift_comment_rating=4)â] - [â[đ Critical, fixing now](https://www.sonatype.com/lift-comment-rating?comment=405095747&lift_comment_rating=5)â]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org