dsmiley commented on code in PR #4799:
URL: https://github.com/apache/solr/pull/4799#discussion_r3837133597
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/SolrStream.java:
##########
@@ -68,18 +69,52 @@ public class SolrStream extends TupleStream {
private transient SolrClientCache clientCache;
private transient boolean doCloseCache;
+ // TODO SOLR-17995 proposes that we should deprecate this constructor in
favor of one of the other
+ // constructors that requires users to provide the core as an explicit
parameter
/**
- * @param baseUrl Base URL of the stream.
- * @param params Map<String, String> of parameters
+ * @param baseUrl URL of the Solr core or collection to query, typically of
the form
+ * "http://host:8983/solr/myCore";.
+ * @param params query-parameters sent with the streaming request
*/
public SolrStream(String baseUrl, SolrParams params) {
this.baseUrl = baseUrl;
this.params = params;
}
- SolrStream(String baseUrl, SolrParams params, String core) {
- this(baseUrl, params);
+ // TODO SOLR-17995 proposes that we should deprecate this constructor in
favor of one of the other
+ // constructors that requires users to provide the core as an explicit
parameter
+ /**
+ * @param baseUrl URL of the Solr core or collection to query, typically of
the form
+ * "http://host:8983/solr/myCore";.
+ * @param path the request handler path to query (e.g. "/export"). If not
provided, defaults to
+ * "/select".
+ * @param params query-parameters sent with the streaming request
+ */
+ public SolrStream(String baseUrl, String path, SolrParams params) {
+ this(baseUrl, null, path, params);
+ }
+
+ /**
+ * @param baseUrl the Solr node's "base" URL (i.e. no core or collection in
the path
+ * @param params query-parameters sent with the streaming request
+ * @param core the name of the collection or core to query; must be hosted
at {@code baseUrl}
+ */
+ public SolrStream(String baseUrl, SolrParams params, String core) {
Review Comment:
I strongly think ordering matters. core has no business being after params.
##########
solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/stream/CloudAuthStreamTest.java:
##########
@@ -893,11 +854,8 @@ public void
testIndirectDeleteStreamInsufficientCredentials() throws Exception {
final SolrStream solrStream =
new SolrStream(
solrUrl + "/" + path,
Review Comment:
yuck; the "path" above is actually the collection name! Can you please
rename the var and use as a separate param
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/SolrStream.java:
##########
@@ -68,18 +69,52 @@ public class SolrStream extends TupleStream {
private transient SolrClientCache clientCache;
private transient boolean doCloseCache;
+ // TODO SOLR-17995 proposes that we should deprecate this constructor in
favor of one of the other
+ // constructors that requires users to provide the core as an explicit
parameter
/**
- * @param baseUrl Base URL of the stream.
- * @param params Map<String, String> of parameters
+ * @param baseUrl URL of the Solr core or collection to query, typically of
the form
+ * "http://host:8983/solr/myCore";.
+ * @param params query-parameters sent with the streaming request
*/
public SolrStream(String baseUrl, SolrParams params) {
this.baseUrl = baseUrl;
this.params = params;
}
- SolrStream(String baseUrl, SolrParams params, String core) {
- this(baseUrl, params);
+ // TODO SOLR-17995 proposes that we should deprecate this constructor in
favor of one of the other
+ // constructors that requires users to provide the core as an explicit
parameter
+ /**
+ * @param baseUrl URL of the Solr core or collection to query, typically of
the form
Review Comment:
baseUrl is a poor name; it should be collectionSolrUrl perhaps. It's not
"base"; that would be to Solr itself.
##########
solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/stream/CloudAuthStreamTest.java:
##########
@@ -857,8 +816,10 @@ public void testIndirectDeleteStream() throws Exception {
final SolrStream solrStream =
new SolrStream(
- solrUrl + "/" + COLLECTION_X, // NOTE: X route
- params("qt", "/stream", "expr", expr));
+ solrUrl,
+ COLLECTION_X,
+ "/stream", // NOTE: X route
Review Comment:
misplaced comment (please handle the rest)
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/SolrStream.java:
##########
@@ -68,18 +69,52 @@ public class SolrStream extends TupleStream {
private transient SolrClientCache clientCache;
private transient boolean doCloseCache;
+ // TODO SOLR-17995 proposes that we should deprecate this constructor in
favor of one of the other
+ // constructors that requires users to provide the core as an explicit
parameter
/**
- * @param baseUrl Base URL of the stream.
- * @param params Map<String, String> of parameters
+ * @param baseUrl URL of the Solr core or collection to query, typically of
the form
+ * "http://host:8983/solr/myCore";.
+ * @param params query-parameters sent with the streaming request
*/
public SolrStream(String baseUrl, SolrParams params) {
this.baseUrl = baseUrl;
this.params = params;
}
- SolrStream(String baseUrl, SolrParams params, String core) {
- this(baseUrl, params);
+ // TODO SOLR-17995 proposes that we should deprecate this constructor in
favor of one of the other
+ // constructors that requires users to provide the core as an explicit
parameter
+ /**
+ * @param baseUrl URL of the Solr core or collection to query, typically of
the form
+ * "http://host:8983/solr/myCore";.
+ * @param path the request handler path to query (e.g. "/export"). If not
provided, defaults to
+ * "/select".
+ * @param params query-parameters sent with the streaming request
+ */
+ public SolrStream(String baseUrl, String path, SolrParams params) {
+ this(baseUrl, null, path, params);
+ }
+
+ /**
+ * @param baseUrl the Solr node's "base" URL (i.e. no core or collection in
the path
+ * @param params query-parameters sent with the streaming request
+ * @param core the name of the collection or core to query; must be hosted
at {@code baseUrl}
+ */
+ public SolrStream(String baseUrl, SolrParams params, String core) {
+ this(baseUrl, core, null, params);
+ }
+
+ /**
+ * @param baseUrl the Solr node's "base" URL (i.e. no core or collection in
the path
+ * @param core the name of the collection or core to query; must be hosted
at {@code baseUrl}
+ * @param path the request handler path to query (e.g. "/export"). If not
provided, defaults to
+ * "/select".
+ * @param params query-parameters sent with the streaming request
+ */
+ public SolrStream(String baseUrl, String core, String path, SolrParams
params) {
+ this.baseUrl = baseUrl;
this.core = core;
+ this.params = params;
+ this.path = path != null ? path : "/select";
Review Comment:
gonna be nit-picky here and ask you to move the path initialization above
params. And likewise move the field definition up likewise.
##########
solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/stream/CloudAuthStreamTest.java:
##########
@@ -398,8 +383,9 @@ public void testIndirectUpdateStream() throws Exception {
final SolrStream solrStream =
new SolrStream(
- solrUrl + "/" + COLLECTION_Y, // NOTE: Y route
- params("qt", "/stream", "expr", expr));
+ solrUrl + "/" + COLLECTION_Y,
+ "/stream", // NOTE: Y route
Review Comment:
the "y route" comment was intended to be on the collection
##########
solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/stream/CloudAuthStreamTest.java:
##########
@@ -833,8 +791,9 @@ public void testIndirectDeleteStream() throws Exception {
final SolrStream solrStream =
new SolrStream(
- solrUrl + "/" + COLLECTION_Y, // NOTE: Y route
- params("qt", "/stream", "expr", expr));
+ solrUrl + "/" + COLLECTION_Y,
+ "/stream", // NOTE: Y route
Review Comment:
misplaced comment
##########
solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/stream/CloudAuthStreamTest.java:
##########
@@ -398,8 +383,9 @@ public void testIndirectUpdateStream() throws Exception {
final SolrStream solrStream =
new SolrStream(
- solrUrl + "/" + COLLECTION_Y, // NOTE: Y route
- params("qt", "/stream", "expr", expr));
+ solrUrl + "/" + COLLECTION_Y,
Review Comment:
I think we can provide the collection as a separate param
##########
solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/stream/CloudAuthStreamTest.java:
##########
@@ -833,8 +791,9 @@ public void testIndirectDeleteStream() throws Exception {
final SolrStream solrStream =
new SolrStream(
- solrUrl + "/" + COLLECTION_Y, // NOTE: Y route
- params("qt", "/stream", "expr", expr));
+ solrUrl + "/" + COLLECTION_Y,
Review Comment:
I think we can provide the collection as a separate param
##########
solr/solrj-streaming/src/test/org/apache/solr/client/solrj/io/stream/CloudAuthStreamTest.java:
##########
@@ -418,8 +404,10 @@ public void testIndirectUpdateStream() throws Exception {
final SolrStream solrStream =
new SolrStream(
- solrUrl + "/" + COLLECTION_X, // NOTE: X route
- params("qt", "/stream", "expr", expr));
+ solrUrl,
+ COLLECTION_X,
+ "/stream", // NOTE: X route
Review Comment:
misplaced comment; put on collection
##########
solr/solrj-streaming/src/java/org/apache/solr/client/solrj/io/stream/SolrStream.java:
##########
@@ -68,18 +69,52 @@ public class SolrStream extends TupleStream {
private transient SolrClientCache clientCache;
private transient boolean doCloseCache;
+ // TODO SOLR-17995 proposes that we should deprecate this constructor in
favor of one of the other
+ // constructors that requires users to provide the core as an explicit
parameter
/**
- * @param baseUrl Base URL of the stream.
- * @param params Map<String, String> of parameters
+ * @param baseUrl URL of the Solr core or collection to query, typically of
the form
+ * "http://host:8983/solr/myCore";.
+ * @param params query-parameters sent with the streaming request
*/
public SolrStream(String baseUrl, SolrParams params) {
this.baseUrl = baseUrl;
this.params = params;
}
- SolrStream(String baseUrl, SolrParams params, String core) {
- this(baseUrl, params);
+ // TODO SOLR-17995 proposes that we should deprecate this constructor in
favor of one of the other
+ // constructors that requires users to provide the core as an explicit
parameter
+ /**
+ * @param baseUrl URL of the Solr core or collection to query, typically of
the form
+ * "http://host:8983/solr/myCore";.
+ * @param path the request handler path to query (e.g. "/export"). If not
provided, defaults to
+ * "/select".
+ * @param params query-parameters sent with the streaming request
+ */
+ public SolrStream(String baseUrl, String path, SolrParams params) {
Review Comment:
This method signature is extremely ambiguous. Maybe "path" should actually
be "collectionOrPath" determined by whether it starts with a forward slash?
--
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]
