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: [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]
