mlbiscoc commented on code in PR #3249:
URL: https://github.com/apache/solr/pull/3249#discussion_r1986281671
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -230,43 +214,41 @@ private SimpleOrderedMap<?> submitClusterStateRequest(
}
@Override
- public Set<String> getLiveNodes() {
+ public synchronized Set<String> getLiveNodes() {
+ // synchronized because there's no value in multiple doing this at the
same time
if (TimeUnit.SECONDS.convert((System.nanoTime() - liveNodesTimestamp),
TimeUnit.NANOSECONDS)
- > getCacheTimeout()) {
+ <= getCacheTimeout()) {
+ return this.liveNodes; // cached copy is fresh enough
+ }
+ if (liveNodes != null) { // thus we've fetched liveNodes previously
Review Comment:
Doesn't adding this mean we never update the node list after the first
`connect` using the existing liveNodes attribute? (assuming liveNodes populates
successfully upon connection to Solr). Now it will only ever update the node
list by fetching from configured nodes.
What if the only liveNode is a node not in the configured list and the cache
timeout hit, then the node would never be fetched from because it isn't the in
configured list.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -78,28 +78,12 @@ public void init(List<String> solrUrls) throws Exception {
}
})
.collect(Collectors.toList());
+ this.urlScheme = this.configuredNodes.get(0).getProtocol();
Review Comment:
Looking at this now and just noticing, this seems a bit awkward. The
`urlScheme` is set based on the first url in the list for http/https but before
it was the last. But what if there is a mix of http/https? Not sure if it was
intentional. It seems there shouldn't be a case where there is a mix of
http/https urls in the configured list but depending on the ordering now, some
urls may or may not work with `Utils.getBaseUrlForNodeName` when getting the
live nodes list.
What do you think of throwing an exception if the urls are mixed with
http/https and enforce a single protocol?
--
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]
