You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "dsmiley (via GitHub)" <gi...@apache.org> on 2025/03/16 23:23:39 UTC
[PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
dsmiley opened a new pull request, #3270:
URL: https://github.com/apache/solr/pull/3270
Is not necessarily a SolrResponse anymore. Helps V2 API and reduces presence of NamedList.
https://issues.apache.org/jira/browse/SOLR-17705
> (Two theme alignments here: V2 API migration, and NamedList reduction)
SolrJ SolrRequest has a parameterized type of its response type that is bound to be of type SolrResponse. SolrResponse isn't an interesting class; it just holds the "response" NamedList, has the elapsed time, and has a utility method to extract an exception from the named list (maybe shouldn't be there...). The most central aspect of what little there is, is the NamedList, which is obtainable from all SolrRequests and is more omnipresent than it'd ideally be (see SIP-22 for reduction). I'd like to change this parameterized type to be unbounded, thus could return anything that isn't necessarily a subclass of SolrResponse. This is more flexible (useful for V2), and it lessens the insistence on responses having a NamedList. It's up to the SolrRequest subclass to create the response, populating it from the internal NamedList passed from the SolrClient's request method. This already happens somewhat in SolrRequest.createResponse (protected), but would need some adjustments in my
proposal here. Our V2 "api" module has classes in which SolrJerseyResponse is the base class of API methods, which is not a subclass of SolrResponse, so there's additional wrapping indirection that could go away with this change.
--
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
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3270:
URL: https://github.com/apache/solr/pull/3270#discussion_r2006265507
##########
solr/test-framework/src/java/org/apache/solr/cloud/ConfigRequest.java:
##########
@@ -53,7 +53,7 @@ public RequestWriter.ContentWriter getContentWriter(String expectedType) {
}
@Override
- public SolrResponse createResponse(SolrClient client) {
+ public SolrResponse createResponse(NamedList client) {
Review Comment:
Indeed; you are really paying attention here :-). Normally I have generics figured out but this was quite a puzzle. To my human eyes, the `<Object>` should be understood but javac was confused about whether this was actually an Override or not. After fiddling with it, an LLM gave this suggestion, which seems fine. I should add a comment including the error javac gave.
--
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
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #3270:
URL: https://github.com/apache/solr/pull/3270#issuecomment-2744501067
Btw - I really appreciated the preliminary GH comments ([like this one](https://github.com/apache/solr/pull/3270#discussion_r1997765212)) you put in yourself to ease the review process. Haven't noticed that sort of thing before, but I'm gonna try picking up the habit as it was a huge help from reviewer side on this one 👍
--
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
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley merged PR #3270:
URL: https://github.com/apache/solr/pull/3270
--
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
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3270:
URL: https://github.com/apache/solr/pull/3270#discussion_r2006358760
##########
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##########
@@ -256,12 +257,15 @@ public RequestWriter.ContentWriter getContentWriter(String expectedType) {
public final T process(SolrClient client, String collection)
throws SolrServerException, IOException {
long startNanos = System.nanoTime();
- T res = createResponse(client);
var namedList = client.request(this, collection);
- res.setResponse(namedList);
long endNanos = System.nanoTime();
- res.setElapsedTime(TimeUnit.NANOSECONDS.toMillis(endNanos - startNanos));
- return res;
+ final T typedResponse = createResponse(namedList);
+ // SolrResponse is pre-V2 API
+ if (typedResponse instanceof SolrResponse res) {
+ res.setResponse(namedList); // TODO insist createResponse does this ?
+ res.setElapsedTime(TimeUnit.NANOSECONDS.toMillis(endNanos - startNanos));
+ }
Review Comment:
At the start, I considered maybe the V2 API's responses should subclass SolrResponse. One stumbling block is that those responses are in the "api" module, which doesn't have a backwards dependency on SolrJ where SolrResponse is. Instead of tackling that, I questioned do we really want it anyway. I think we don't. I like the open-ended flexibility allowed in this PR. We could have a custom request that wants to return an Integer and it'd return exactly that; not obscured in some other layer to obtain it out of.
The only extra/contextual info here is the elapsed time. If you think there's something else then please share what that is. So on the time... I dunno. Yeah it's nice to have I guess.... but honestly I don't use it. I care about the time in observability (logs, metrics, tracing), not actually in the API. Obviously the user/caller could easily measure the time if they wanted to.
--
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
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3270:
URL: https://github.com/apache/solr/pull/3270#discussion_r2006260069
##########
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##########
@@ -240,9 +241,9 @@ public RequestWriter.ContentWriter getContentWriter(String expectedType) {
/**
* Create a new SolrResponse to hold the response from the server
*
- * @param client the {@link SolrClient} the request will be sent to
+ * @param namedList the {@link SolrClient} the request will be sent to
*/
- protected abstract T createResponse(SolrClient client);
+ protected abstract T createResponse(NamedList<Object> namedList);
Review Comment:
Right; no impl uses it yet; the V2 API certainly will; maybe I'll show that this weekend. I could change the V1 APIs to use it; it'd increase the scope of this PR a good deal but for the better. On the other hand, there's a case for leaving them be; we're moving on to V2 after all. If you like, I could do this to one response to show you what that looks like. Maybe I'll just do that now; it's not hard. Other than that; I think what I already wrote (which is a lot) establishes the context. If it's still not clear, you might find checking out the PR to see it naturally and not as a diff.
--
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
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #3270:
URL: https://github.com/apache/solr/pull/3270#discussion_r2007952602
##########
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##########
@@ -240,9 +241,9 @@ public RequestWriter.ContentWriter getContentWriter(String expectedType) {
/**
* Create a new SolrResponse to hold the response from the server
*
- * @param client the {@link SolrClient} the request will be sent to
+ * @param namedList the {@link SolrClient} the request will be sent to
*/
- protected abstract T createResponse(SolrClient client);
+ protected abstract T createResponse(NamedList<Object> namedList);
Review Comment:
> I could change the V1 APIs to use it; it'd increase the scope of this PR a good deal but for the better
If you want to, great. But don't expand the scope that much on my account! Your answer is plenty explanation for my sake.
> Right; no impl uses it yet; the V2 API certainly will
Dang, don't love the irony of the v2 API being the main cause of a new NamedList usage.
It'd be really awesome if this could take the entire `InputStream`, and have Jackson parse directly from that rather than use the NamedList intermediary. But I guess that's not possible because `process` calls `request` which needs to return a NL in turn.... *sigh*
Ignore me, just thinking aloud here...
--
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
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #3270:
URL: https://github.com/apache/solr/pull/3270#issuecomment-2794242747
Oops; sorry about that that! Thanks @HoustonPutman ! We need a better change-log system.
--
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
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #3270:
URL: https://github.com/apache/solr/pull/3270#issuecomment-2794237822
I added back the changelog entries that this removed.
--
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
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #3270:
URL: https://github.com/apache/solr/pull/3270#issuecomment-2741398533
I'm not going to delay the V2 work that will use this; I could even start a branch off of this one to show the work.
--
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
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #3270:
URL: https://github.com/apache/solr/pull/3270#issuecomment-2741817502
I added some SolrRequest javadocs for clarification; I think you'll like it.
I ultimately didn't customize any createResponse implementation because we universally use V1 SolrResponse, which has it's setResponse(NamedList) called, which means attempting to customize will result in the response being set twice. I was tempted to change in V2 but that quickly snowballs the scope creep which I want to avoid. Again, I'll show something later to show it works.
--
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
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3270:
URL: https://github.com/apache/solr/pull/3270#discussion_r2006552011
##########
solr/test-framework/src/java/org/apache/solr/cloud/ConfigRequest.java:
##########
@@ -53,7 +53,7 @@ public RequestWriter.ContentWriter getContentWriter(String expectedType) {
}
@Override
- public SolrResponse createResponse(SolrClient client) {
+ public SolrResponse createResponse(NamedList client) {
Review Comment:
Glad you prodded... I fiddled with it a bit more and easily got around it after all :-)
--
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
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #3270:
URL: https://github.com/apache/solr/pull/3270#issuecomment-2789924268
Will merge tonight, after resolving CHANGES.txt.
--
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
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #3270:
URL: https://github.com/apache/solr/pull/3270#discussion_r2007938592
##########
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##########
@@ -256,12 +257,15 @@ public RequestWriter.ContentWriter getContentWriter(String expectedType) {
public final T process(SolrClient client, String collection)
throws SolrServerException, IOException {
long startNanos = System.nanoTime();
- T res = createResponse(client);
var namedList = client.request(this, collection);
- res.setResponse(namedList);
long endNanos = System.nanoTime();
- res.setElapsedTime(TimeUnit.NANOSECONDS.toMillis(endNanos - startNanos));
- return res;
+ final T typedResponse = createResponse(namedList);
+ // SolrResponse is pre-V2 API
+ if (typedResponse instanceof SolrResponse res) {
+ res.setResponse(namedList); // TODO insist createResponse does this ?
+ res.setElapsedTime(TimeUnit.NANOSECONDS.toMillis(endNanos - startNanos));
+ }
Review Comment:
OK, sounds good 👍
--
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
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #3270:
URL: https://github.com/apache/solr/pull/3270#discussion_r2006039167
##########
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##########
@@ -256,12 +257,15 @@ public RequestWriter.ContentWriter getContentWriter(String expectedType) {
public final T process(SolrClient client, String collection)
throws SolrServerException, IOException {
long startNanos = System.nanoTime();
- T res = createResponse(client);
var namedList = client.request(this, collection);
- res.setResponse(namedList);
long endNanos = System.nanoTime();
- res.setElapsedTime(TimeUnit.NANOSECONDS.toMillis(endNanos - startNanos));
- return res;
+ final T typedResponse = createResponse(namedList);
+ // SolrResponse is pre-V2 API
+ if (typedResponse instanceof SolrResponse res) {
+ res.setResponse(namedList); // TODO insist createResponse does this ?
+ res.setElapsedTime(TimeUnit.NANOSECONDS.toMillis(endNanos - startNanos));
+ }
Review Comment:
There's nothing about v2 that makes "elapsed time" less relevant - that makes sense to have for all requests, seemingly. And I'd bet that there are other concerns and methods in a similar position.
Is there any argument to loosening the type bounds, but not entirely, and still bounding responses to implement some minimal interface with default method implementations for basic functionality like "elapsed time"?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##########
@@ -240,9 +241,9 @@ public RequestWriter.ContentWriter getContentWriter(String expectedType) {
/**
* Create a new SolrResponse to hold the response from the server
*
- * @param client the {@link SolrClient} the request will be sent to
+ * @param namedList the {@link SolrClient} the request will be sent to
*/
- protected abstract T createResponse(SolrClient client);
+ protected abstract T createResponse(NamedList<Object> namedList);
Review Comment:
At a glance I can't find a single implementation of this method that actually uses the provided NamedList? Am I missing a few exceptions? I know we'd love to cut down on NamedList usage - why include it in the signature at all?
##########
solr/test-framework/src/java/org/apache/solr/cloud/ConfigRequest.java:
##########
@@ -53,7 +53,7 @@ public RequestWriter.ContentWriter getContentWriter(String expectedType) {
}
@Override
- public SolrResponse createResponse(SolrClient client) {
+ public SolrResponse createResponse(NamedList client) {
Review Comment:
[0] You used `NamedList<Object>` everywhere else in this method sig - was it just oversight here?
--
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
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3270:
URL: https://github.com/apache/solr/pull/3270#discussion_r2008132446
##########
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##########
@@ -240,9 +241,9 @@ public RequestWriter.ContentWriter getContentWriter(String expectedType) {
/**
* Create a new SolrResponse to hold the response from the server
*
- * @param client the {@link SolrClient} the request will be sent to
+ * @param namedList the {@link SolrClient} the request will be sent to
*/
- protected abstract T createResponse(SolrClient client);
+ protected abstract T createResponse(NamedList<Object> namedList);
Review Comment:
RE NamedList: this PR technically adds an API endpoint (for those that would create custom requests), but it sort of removes another, albeit not actually :-). `SolrResponse.setResponse(NamedList)` is the existing spot that populates the response. This PR instead _suggests_ constructor based population, which is what will happen for the V2 side and any other API we (or a user) makes to leverage a response that is not SolrResponse based. I'm choosing to leave `SolrResponse.setResponse(NamedList)` as-is because it doesn't seem worth disturbing it. It's not the future.
--
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
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3270:
URL: https://github.com/apache/solr/pull/3270#discussion_r2006261071
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/QueryRequest.java:
##########
@@ -61,8 +61,8 @@ public String getPath() {
// ---------------------------------------------------------------------------------
@Override
- protected QueryResponse createResponse(SolrClient client) {
- return new QueryResponse(client);
+ protected QueryResponse createResponse(NamedList<Object> namedList) {
+ return new QueryResponse();
Review Comment:
I addressed this in another issue/PR that will merge first.
--
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
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #3270:
URL: https://github.com/apache/solr/pull/3270#issuecomment-2744535206
NP :-). I observed some colleagues at work do a self-review right after posting a PR, and I appreciated it so much (as a reviewer) that I try to do the same for PRs I author for anything non-trivial. And it lessens some desire to have a PR description that _might_ otherwise have gone into lower level details that is best left to inline self-review in-context.
--
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
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #3270:
URL: https://github.com/apache/solr/pull/3270#issuecomment-2743914122
+1 - definitely don't want to explode the scope of this. Progress over perfection.
👍 on the new Javadocs btw
--
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
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3270:
URL: https://github.com/apache/solr/pull/3270#discussion_r1997764947
##########
solr/core/src/test/org/apache/solr/response/transform/TestSubQueryTransformerDistrib.java:
##########
@@ -182,8 +182,7 @@ public void test() throws Exception {
final SolrDocumentList hits;
{
final QueryRequest qr = withBasicAuth(new QueryRequest(params));
- final QueryResponse rsp = new QueryResponse();
- rsp.setResponse(cluster.getSolrClient().request(qr, people + "," + depts));
+ final QueryResponse rsp = qr.process(cluster.getSolrClient(), people + "," + depts);
Review Comment:
just a trivial cleanup
##########
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##########
@@ -256,12 +257,15 @@ public RequestWriter.ContentWriter getContentWriter(String expectedType) {
public final T process(SolrClient client, String collection)
throws SolrServerException, IOException {
long startNanos = System.nanoTime();
- T res = createResponse(client);
var namedList = client.request(this, collection);
- res.setResponse(namedList);
long endNanos = System.nanoTime();
- res.setElapsedTime(TimeUnit.NANOSECONDS.toMillis(endNanos - startNanos));
- return res;
+ final T typedResponse = createResponse(namedList);
+ // SolrResponse is pre-V2 API
+ if (typedResponse instanceof SolrResponse res) {
+ res.setResponse(namedList); // TODO insist createResponse does this ?
+ res.setElapsedTime(TimeUnit.NANOSECONDS.toMillis(endNanos - startNanos));
+ }
Review Comment:
it's not beautiful but it's practical
##########
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##########
@@ -29,11 +29,12 @@
import org.apache.solr.client.solrj.request.RequestWriter;
import org.apache.solr.common.params.SolrParams;
import org.apache.solr.common.util.ContentStream;
+import org.apache.solr.common.util.NamedList;
/**
* @since solr 1.3
*/
-public abstract class SolrRequest<T extends SolrResponse> implements Serializable {
+public abstract class SolrRequest<T> implements Serializable {
Review Comment:
the principle change of this PR
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/QueryRequest.java:
##########
@@ -61,8 +61,8 @@ public String getPath() {
// ---------------------------------------------------------------------------------
@Override
- protected QueryResponse createResponse(SolrClient client) {
- return new QueryResponse(client);
+ protected QueryResponse createResponse(NamedList<Object> namedList) {
+ return new QueryResponse();
Review Comment:
I overlooked this spot; it uses client as a cache for caching the "DocumentObjectBinder" on the SolrClient. That seems like a bit of a hack to me; should be cached somewhere, maybe even a static global or ThreadLocal. I don't see it as bound to the client instance.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/SolrRequest.java:
##########
@@ -240,9 +241,9 @@ public RequestWriter.ContentWriter getContentWriter(String expectedType) {
/**
* Create a new SolrResponse to hold the response from the server
*
- * @param client the {@link SolrClient} the request will be sent to
+ * @param namedList the {@link SolrClient} the request will be sent to
*/
- protected abstract T createResponse(SolrClient client);
+ protected abstract T createResponse(NamedList<Object> namedList);
Review Comment:
Almost no implementation actually used the SolrClient; that was a weird parameter. But now it's a NamedList, which should be used to not only create the response but populate it too. Admittedly this PR in its initial form as I write this, doesn't do that. It was trivial to add a special case for that (when result implements SolrResponse), which you'll see below.
There's a great deal of irony dawning on me, that the JIRA mentioned reducing NamedList usage and here I add it here. (blush). Okay but this is a protected method and used at an internal spot where the NamedList is a holder that's not avoidable because SolrClient.request (obtained from the ResponseParser) returns a NamedList. My point in JIRA was that SolrRequest's response type isn't necessarily a SolrResponse (SolrResponse exposes a NamedList), and thus from the caller's point of view using V2, they won't see one.
TODO javadoc is wrong.
##########
solr/core/src/test/org/apache/solr/filestore/TestDistribFileStore.java:
##########
@@ -246,7 +246,7 @@ public static <T extends NavigableObject> T assertResponseValues(
public static NavigableObject assertResponseValues(
int repeats, SolrClient client, SolrRequest<?> req, Map<String, Object> vals)
throws Exception {
- Callable<NavigableObject> callable = () -> req.process(client);
+ Callable<NavigableObject> callable = () -> client.request(req);
Review Comment:
switched to request method, which returned NamedList which is a NaviagableObject. `process` now has unbound type and thus could be anything.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/HealthCheckRequest.java:
##########
@@ -55,10 +53,9 @@ public SolrParams getParams() {
}
@Override
- protected HealthCheckResponse createResponse(SolrClient client) {
+ protected HealthCheckResponse createResponse(NamedList<Object> namedList) {
// TODO: Accept requests w/ CloudSolrClient while ensuring that the request doesn't get routed
// to an unintended recipient.
- assert client instanceof BaseHttpSolrClient || client instanceof HttpSolrClientBase;
Review Comment:
this was the only spot that used `SolrClient`
--
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