epugh commented on code in PR #3271:
URL: https://github.com/apache/solr/pull/3271#discussion_r1998680285
##########
solr/core/src/test/org/apache/solr/cloud/DistribJoinFromCollectionTest.java:
##########
@@ -147,7 +147,7 @@ private void testJoins(String toColl, String fromColl,
String toDocId, boolean i
+ fromQ;
QueryRequest qr =
new QueryRequest(params("collection", toColl, "q", joinQ, "fl",
"id,get_s,score"));
- QueryResponse rsp = new QueryResponse(client.request(qr), client);
+ QueryResponse rsp = qr.process(client);
Review Comment:
I love all these changes, much clearer....
##########
solr/CHANGES.txt:
##########
@@ -160,6 +160,9 @@ Other Changes
thus BinaryQueryResponseWriter is gone. Added TextQueryResponseWriter for
the text ones.
Stopped using RawResponseWriter when not needed. (David Smiley)
+* SOLR-17706: SolrJ DocumentObjectBinder is now a global singleton via an
INSTANCE field and that
Review Comment:
"and is pluggable". Out of curisotiy, do you want to say it is pluggable
if it hasn't been tested?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/beans/DocumentObjectBinder.java:
##########
@@ -41,13 +42,22 @@
import org.apache.solr.common.util.SuppressForbidden;
/**
- * A class to map objects to and from solr documents.
+ * Maps objects to and from Solr documents.
Review Comment:
nice!
##########
solr/solrj/src/java/org/apache/solr/client/solrj/response/QueryResponse.java:
##########
@@ -643,9 +631,7 @@ public List<FacetField> getLimitingFacets() {
}
public <T> List<T> getBeans(Class<T> type) {
- return solrClient == null
- ? new DocumentObjectBinder().getBeans(type, _results)
- : solrClient.getBinder().getBeans(type, _results);
+ return DocumentObjectBinder.INSTANCE.getBeans(type, _results);
Review Comment:
much nicer! Love there isn't two ways to get it...
##########
solr/solrj/src/java/org/apache/solr/client/solrj/response/QueryResponse.java:
##########
@@ -102,21 +101,10 @@ public class QueryResponse extends SolrResponseBase {
private Map<String, Object> _debugMap = null;
private Map<String, Object> _explainMap = null;
- // utility variable used for automatic binding -- it should not be serialized
- private final transient SolrClient solrClient;
+ public QueryResponse() {}
- public QueryResponse() {
- solrClient = null;
- }
-
- /** Utility constructor to set the solrServer and namedList */
- public QueryResponse(NamedList<Object> res, SolrClient solrClient) {
+ public QueryResponse(NamedList<Object> res) {
Review Comment:
I love this, but also, does this need to be deprecated and or flagged?
Seems like a big method chagne?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/beans/DocumentObjectBinder.java:
##########
@@ -126,18 +136,12 @@ private void addChild(Object obj, DocField field,
SolrInputDocument doc) {
}
private List<DocField> getDocFields(Class<?> clazz) {
Review Comment:
much shorter, and actually is easier to read...
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/QueryRequest.java:
##########
@@ -62,7 +62,7 @@ public String getPath() {
@Override
protected QueryResponse createResponse(SolrClient client) {
- return new QueryResponse(client);
+ return new QueryResponse();
Review Comment:
does the fact that this takes in a `client` but does not use it need to be
documented? Just reading it, I would think "thats weried"..
--
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]
