Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
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 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
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 ``
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
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 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
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 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
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 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
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 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` 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-17705 SolrRequest's type is now unbounded [solr]
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 implements
Serializable {
+public abstract class SolrRequest 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 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 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
assertResponseValues(
public static NavigableObject assertResponseValues(
int repeats, SolrClient client, SolrRequest req, Map
vals)
throws Exception {
-Callable callable = () -> req.process(client);
+Callable 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.
##
