epugh commented on code in PR #2229:
URL: https://github.com/apache/solr/pull/2229#discussion_r1469954419
##########
solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java:
##########
@@ -237,8 +237,10 @@ public Map<String, SolrPackageInstance>
getPackagesDeployed(String collection) {
NamedList<Object> result =
solrClient.request(
new GenericSolrRequest(
- SolrRequest.METHOD.GET,
- PackageUtils.getCollectionParamsPath(collection) +
"/PKG_VERSIONS"));
+ SolrRequest.METHOD.GET,
+ PackageUtils.getCollectionParamsPath(collection) +
"/PKG_VERSIONS")
+ .setRequiresCollection(
+ false) /* Making a collection request, but already baked
into path */);
Review Comment:
interesting, is this a place we could try and improve things to not have
this logic? Seems a shame to have a collection request and state 'but we
don't require it'....?
##########
solr/core/src/java/org/apache/solr/cli/ExportTool.java:
##########
@@ -620,7 +622,7 @@ boolean exportDocsFromCore() throws IOException,
SolrServerException {
try (SolrClient client = SolrCLI.getSolrClient(baseurl, credentials)) {
expectedDocs = getDocCount(replica.getCoreName(), client, query);
- GenericSolrRequest request;
+ QueryRequest request;
Review Comment:
I like that you were able to move away from the `GenericSolrRequest` here!
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/GenericSolrRequest.java:
##########
Review Comment:
ooh, docs!
##########
solr/core/src/java/org/apache/solr/packagemanager/PackageUtils.java:
##########
@@ -155,9 +155,12 @@ public static String getFileFromJarsAsString(List<Path>
jars, String filename) {
}
/** Returns JSON string from a given Solr URL */
- public static String getJsonStringFromUrl(SolrClient client, String path,
SolrParams params) {
+ public static String getJsonStringFromUrl(
Review Comment:
I wonder if it would be simpler to just have "getJsonStringFromCollection"
and "getJsonStringFromSolr" or something? After all, it's not a generic
"getJsonStringFromUrl" where I can pass in any URL, despite what the name of
the method appears to suggest...! Then we could skip passing a magic boolean
around...
##########
solr/solrj/src/java/org/apache/solr/client/solrj/request/GenericSolrRequest.java:
##########
@@ -29,16 +29,48 @@ public class GenericSolrRequest extends
SolrRequest<SimpleSolrResponse> {
public SolrParams params;
public SimpleSolrResponse response = new SimpleSolrResponse();
public ContentWriter contentWriter;
+ public boolean requiresCollection;
+ /**
+ * @param m the HTTP method to use for this request
+ * @param path the HTTP path to use for this request. If users are making a
collection-aware
+ * request (i.e. {@link #setRequiresCollection(boolean)} is called with
'true'), only the
+ * section of the API path following the collection or core should be
provided here.
+ */
public GenericSolrRequest(METHOD m, String path) {
this(m, path, new ModifiableSolrParams());
}
+ /**
+ * @param m the HTTP method to use for this request
+ * @param path the HTTP path to use for this request. If users are making a
collection-aware
+ * request (i.e. {@link #setRequiresCollection(boolean)} is called with
'true'), only the
+ * section of the API path following the collection or core should be
provided here.
+ * @param params query parameter names and values for making this request.
+ */
public GenericSolrRequest(METHOD m, String path, SolrParams params) {
super(m, path);
this.params = params;
}
+ /**
+ * Determines whether the SolrRequest should use a default collection or
core from the client
+ *
+ * <p>Should generally be 'true' whenever making a request to a particular
collection or core, and
+ * 'false' otherwise.
Review Comment:
Wish `generally` could be `always` !
##########
solr/core/src/test/org/apache/solr/security/BasicAuthIntegrationTest.java:
##########
@@ -330,7 +330,7 @@ public void testBasicAuth() throws Exception {
assertPkiAuthMetricsMinimums(3, 3, 0, 0, 0, 0);
// Query that succeeds
- GenericSolrRequest req = new GenericSolrRequest(SolrRequest.METHOD.GET,
"/select", params);
+ final var req = new QueryRequest(params);
Review Comment:
Yay!
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -1013,7 +1013,8 @@ public void testGetRawStream() throws Exception {
getBaseUrl() + "/debug/foo", DEFAULT_CONNECTION_TIMEOUT,
DEFAULT_CONNECTION_TIMEOUT)
.build()) {
GenericSolrRequest req =
- new GenericSolrRequest(SolrRequest.METHOD.GET, "/select",
params("q", "*:*"));
+ new GenericSolrRequest(SolrRequest.METHOD.GET, "/select",
params("q", "*:*"))
Review Comment:
Could this be replaced with a `QueryRequest` instead?
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
