mlbiscoc commented on code in PR #2935:
URL: https://github.com/apache/solr/pull/2935#discussion_r1920793611
##########
solr/solrj/src/java/org/apache/solr/common/util/Utils.java:
##########
@@ -786,6 +789,20 @@ public static String getBaseUrlForNodeName(
return urlScheme + "://" + hostAndPort + "/" + (isV2 ? "api" : "solr");
}
+ /**
+ * Construct base Solr URL to a Solr node name
+ *
+ * @param solrUrl Given a base Solr URL string (e.g.,
'https://app-node-1:8983/solr')
+ * @return URL that looks like {@code app-node-1:8983_solr}
+ * @throws MalformedURLException if the provided URL string is malformed
+ * @throws URISyntaxException if the provided URL string could not be parsed
as a URI reference.
+ */
+ public static String getNodeNameFromSolrUrl(String solrUrl)
+ throws MalformedURLException, URISyntaxException {
+ URL url = new URI(solrUrl).toURL();
+ return url.getAuthority() + url.getPath().replace('/', '_');
+ }
Review Comment:
I couldn't find anywhere where this is a function like this (Maybe you
someone knows?). Regardless, I think if it exists somewhere, it should sit here
because the `getBaseUrlForNodeName` function right above it does the conversion
`nodeName->Url` and putting `url->nodeName` right below it makes sense to me.
Added unit tests and just added unit tests for `getBaseUrlForNodeName` because
it didn't exist before.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -226,17 +232,15 @@ public Set<String> getLiveNodes() {
}
if (TimeUnit.SECONDS.convert((System.nanoTime() - liveNodesTimestamp),
TimeUnit.NANOSECONDS)
> getCacheTimeout()) {
- for (String nodeName : liveNodes) {
- String baseUrl = Utils.getBaseUrlForNodeName(nodeName, urlScheme);
- try (SolrClient client = getSolrClient(baseUrl)) {
- Set<String> liveNodes = fetchLiveNodes(client);
- this.liveNodes = (liveNodes);
- liveNodesTimestamp = System.nanoTime();
- return liveNodes;
- } catch (Exception e) {
- log.warn("Attempt to fetch cluster state from {} failed.", baseUrl,
e);
- }
- }
+
+ if (updateLiveNodes(liveNodes)) return this.liveNodes;
+
+ log.warn(
+ "Attempt to fetch cluster state from all known live nodes {} failed.
Trying backup nodes",
+ liveNodes);
+
+ if (updateLiveNodes(backupNodes)) return this.liveNodes;
Review Comment:
Backup is checked if liveNodes is exhausted. There are a few places that
liveNodes is used for fetching but backup isn't now. Was thinking of adding it,
but that requires a larger refactor from just this bug and writing additional
tests. Happy to do it though.
--
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]
