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