dsmiley commented on code in PR #2935:
URL: https://github.com/apache/solr/pull/2935#discussion_r1911044868
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -229,10 +236,9 @@ > getCacheTimeout()) {
for (String nodeName : liveNodes) {
Review Comment:
If we exhaust liveNodes, shouldn't we then try the initial configured nodes,
and then only failing that, throw an exception?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -413,6 +418,30 @@ public String getQuorumHosts() {
return String.join(",", this.liveNodes);
}
+ /** Live nodes should always have the latest set of live nodes but never
remove initial set */
+ private void setLiveNodes(Set<String> nodes) {
+ Set<String> liveNodes = new HashSet<>(nodes);
+ liveNodes.addAll(this.initialNodes);
+ this.liveNodes = Set.copyOf(liveNodes);
+ }
+
+ public Set<String> getNodeNamesFromSolrUrls(List<String> urls)
+ throws URISyntaxException, MalformedURLException {
+ Set<String> set = new HashSet<>();
+ for (String url : urls) {
+ String nodeNameFromSolrUrl = getNodeNameFromSolrUrl(url);
+ set.add(nodeNameFromSolrUrl);
+ }
+ return Collections.unmodifiableSet(set);
Review Comment:
Could easily be converted to a single expression with streams
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -413,6 +418,30 @@ public String getQuorumHosts() {
return String.join(",", this.liveNodes);
}
+ /** Live nodes should always have the latest set of live nodes but never
remove initial set */
+ private void setLiveNodes(Set<String> nodes) {
+ Set<String> liveNodes = new HashSet<>(nodes);
+ liveNodes.addAll(this.initialNodes);
+ this.liveNodes = Set.copyOf(liveNodes);
+ }
+
+ public Set<String> getNodeNamesFromSolrUrls(List<String> urls)
+ throws URISyntaxException, MalformedURLException {
+ Set<String> set = new HashSet<>();
+ for (String url : urls) {
+ String nodeNameFromSolrUrl = getNodeNameFromSolrUrl(url);
+ set.add(nodeNameFromSolrUrl);
+ }
+ return Collections.unmodifiableSet(set);
+ }
+
+ /** URL to cluster state node name (http://127.0.0.1:12345/solr to
127.0.0.1:12345_solr) */
+ public String getNodeNameFromSolrUrl(String solrUrl)
+ throws MalformedURLException, URISyntaxException {
+ URL url = new URI(solrUrl).toURL();
+ return url.getAuthority() + url.getPath().replace('/', '_');
+ }
Review Comment:
can you find other code in Solr doing this? Surely it's somewhere.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -413,6 +418,30 @@ public String getQuorumHosts() {
return String.join(",", this.liveNodes);
}
+ /** Live nodes should always have the latest set of live nodes but never
remove initial set */
+ private void setLiveNodes(Set<String> nodes) {
+ Set<String> liveNodes = new HashSet<>(nodes);
+ liveNodes.addAll(this.initialNodes);
+ this.liveNodes = Set.copyOf(liveNodes);
+ }
+
+ public Set<String> getNodeNamesFromSolrUrls(List<String> urls)
+ throws URISyntaxException, MalformedURLException {
+ Set<String> set = new HashSet<>();
+ for (String url : urls) {
+ String nodeNameFromSolrUrl = getNodeNameFromSolrUrl(url);
+ set.add(nodeNameFromSolrUrl);
+ }
+ return Collections.unmodifiableSet(set);
+ }
+
+ /** URL to cluster state node name (http://127.0.0.1:12345/solr to
127.0.0.1:12345_solr) */
+ public String getNodeNameFromSolrUrl(String solrUrl)
+ throws MalformedURLException, URISyntaxException {
+ URL url = new URI(solrUrl).toURL();
+ return url.getAuthority() + url.getPath().replace('/', '_');
+ }
Review Comment:
should be a static method that with a unit test
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -413,6 +418,30 @@ public String getQuorumHosts() {
return String.join(",", this.liveNodes);
}
+ /** Live nodes should always have the latest set of live nodes but never
remove initial set */
+ private void setLiveNodes(Set<String> nodes) {
+ Set<String> liveNodes = new HashSet<>(nodes);
+ liveNodes.addAll(this.initialNodes);
+ this.liveNodes = Set.copyOf(liveNodes);
Review Comment:
Why is this 3 lines of extra copy-ing instead of nothing more than:
`this.liveNodes = Set.copyOf(nodes);` ? That is, why are we touching / using
initialNodes at all here?
Maybe an IllegalArgumentException if nodes.isEmpty.
--
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
