epugh commented on code in PR #2473:
URL: https://github.com/apache/solr/pull/2473#discussion_r1609824037
##########
solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java:
##########
@@ -572,7 +572,7 @@ private V2Request postPlugin(Object payload) {
public void waitForAllNodesToSync(String path, Map<String, Object> expected)
throws Exception {
for (JettySolrRunner jettySolrRunner : cluster.getJettySolrRunners()) {
- String baseUrl =
jettySolrRunner.getBaseUrl().toString().replace("/solr", "/api");
+ String baseUrl = jettySolrRunner.getBaseURLV2().toString();
Review Comment:
so many `.toString()` ;-). I wonder... Should we be working more with
URL objects and less with String objects when we build our urls???
##########
solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java:
##########
@@ -260,7 +259,7 @@ public void testModifyPropertiesV2() throws Exception {
checkFooAndBarMeta(aliasName, zkStateReader, "baz", "bam");
String aliasPropertyApi =
- String.format(Locale.ENGLISH, "/api/aliases/%s/properties/%s",
aliasName, "foo");
Review Comment:
that's some funky logic that it's nice to see sorted out....
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java:
##########
@@ -374,7 +374,7 @@ protected HttpRequestBase createMethod(SolrRequest<?>
request, String collection
if (request instanceof V2Request) {
if (System.getProperty("solr.v2RealPath") == null || ((V2Request)
request).isForceV2()) {
- basePath = baseUrl.replace("/solr", "/api");
+ basePath = changeV2RequestEndpoint(baseUrl);
Review Comment:
unfortunately, until we get `HttpSolrClient` gone, we probably should
because it is used in so many places. We really DO need to get the migration
done in Solr 10 so we can cut down on the surface of changes every time we
touch an implementation.
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -1331,12 +1331,24 @@ public ConfigData getSecurityProps(boolean getFresh) {
* Returns the baseURL corresponding to a given node's nodeName -- NOTE:
does not (currently)
* imply that the nodeName (or resulting baseURL) exists in the cluster.
*
- * @lucene.experimental
+ * @param nodeName name of the node
+ * @return url that looks like {@code https://localhost:8983/solr}
*/
public String getBaseUrlForNodeName(final String nodeName) {
return Utils.getBaseUrlForNodeName(nodeName,
getClusterProperty(URL_SCHEME, "http"));
}
+ /**
+ * Returns the V2 baseURL corresponding to a given node's nodeName -- NOTE:
does not (currently)
+ * imply that the nodeName (or resulting baseURL) exists in the cluster.
+ *
+ * @param nodeName name of the node
+ * @return url that looks like {@code https://localhost:8983/api}
+ */
+ public String getBaseUrlV2ForNodeName(final String nodeName) {
+ return Utils.getBaseUrlForNodeName(nodeName,
getClusterProperty(URL_SCHEME, "http"), true);
Review Comment:
is it just me, or does this method look exactly like the one in
`getBaseUrlForNodeName`??? Where do we do the `/api` conversion?
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -1331,12 +1331,24 @@ public ConfigData getSecurityProps(boolean getFresh) {
* Returns the baseURL corresponding to a given node's nodeName -- NOTE:
does not (currently)
* imply that the nodeName (or resulting baseURL) exists in the cluster.
*
- * @lucene.experimental
Review Comment:
nice clean up!
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -1331,12 +1331,24 @@ public ConfigData getSecurityProps(boolean getFresh) {
* Returns the baseURL corresponding to a given node's nodeName -- NOTE:
does not (currently)
* imply that the nodeName (or resulting baseURL) exists in the cluster.
*
- * @lucene.experimental
+ * @param nodeName name of the node
+ * @return url that looks like {@code https://localhost:8983/solr}
*/
public String getBaseUrlForNodeName(final String nodeName) {
return Utils.getBaseUrlForNodeName(nodeName,
getClusterProperty(URL_SCHEME, "http"));
}
+ /**
+ * Returns the V2 baseURL corresponding to a given node's nodeName -- NOTE:
does not (currently)
+ * imply that the nodeName (or resulting baseURL) exists in the cluster.
+ *
+ * @param nodeName name of the node
+ * @return url that looks like {@code https://localhost:8983/api}
+ */
+ public String getBaseUrlV2ForNodeName(final String nodeName) {
+ return Utils.getBaseUrlForNodeName(nodeName,
getClusterProperty(URL_SCHEME, "http"), true);
Review Comment:
Looking up to line 1338
##########
solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java:
##########
@@ -1590,8 +1590,7 @@ private long uploadGivenConfigSet(
Map<?, ?> map =
postDataAndGetResponse(
cluster.getSolrClient(),
-
cluster.getJettySolrRunners().get(0).getBaseUrl().toString().replace("/solr",
"")
- + uriEnding,
+ cluster.getJettySolrRunners().get(0).getBaseURLV2().toString() +
uriEnding,
Review Comment:
is `toString` required? Just wondering if it does it implicitly. (I also
wonder if we do a `.getBaseURLV2().toString()` everywhere, does that tell us
something about the method we are calling!)
--
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
