Copilot commented on code in PR #3673:
URL: https://github.com/apache/solr/pull/3673#discussion_r2364996641
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -213,25 +220,49 @@ private SimpleOrderedMap<?> submitClusterStateRequest(
}
@Override
- public synchronized Set<String> getLiveNodes() {
- // synchronized because there's no value in multiple doing this at the
same time
+ public Set<String> getLiveNodes() {
+ // Use cached liveNodes if cached and still valid
+ if (liveNodes == null
+ || (TimeUnit.SECONDS.convert((System.nanoTime() - liveNodesTimestamp),
TimeUnit.NANOSECONDS)
+ <= getCacheTimeout())) {
Review Comment:
The condition logic is inverted. It should use `>` instead of `<=` to check
if the cached value has expired. Currently, it calls fetchLiveNodes when the
cache is still valid instead of when it's expired.
```suggestion
> getCacheTimeout())) {
```
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/BaseHttpClusterStateProvider.java:
##########
@@ -213,25 +220,49 @@ private SimpleOrderedMap<?> submitClusterStateRequest(
}
@Override
- public synchronized Set<String> getLiveNodes() {
- // synchronized because there's no value in multiple doing this at the
same time
+ public Set<String> getLiveNodes() {
+ // Use cached liveNodes if cached and still valid
+ if (liveNodes == null
+ || (TimeUnit.SECONDS.convert((System.nanoTime() - liveNodesTimestamp),
TimeUnit.NANOSECONDS)
+ <= getCacheTimeout())) {
+ // Do synchronized fetch when there is no cached value or the cached
value is expired
+ fetchLiveNodes(false);
+ }
+ return liveNodes;
+ }
+
+ private synchronized void fetchLiveNodes(boolean force) {
+ if (!liveNodeReloadingScheduled) {
+ // Method is synchronized, so this is safe
+ liveNodeReloadingScheduled = true;
+ liveNodeReloadingService.scheduleWithFixedDelay(
+ () -> fetchLiveNodes(true),
+ (1000L * getCacheTimeout()) / 2,
+ (1000L * getCacheTimeout()) / 2,
Review Comment:
The calculation `(1000L * getCacheTimeout()) / 2` is duplicated. Consider
extracting this into a variable or method to avoid repetition and improve
readability.
```suggestion
long liveNodeReloadDelayMs = (1000L * getCacheTimeout()) / 2;
liveNodeReloadingService.scheduleWithFixedDelay(
() -> fetchLiveNodes(true),
liveNodeReloadDelayMs,
liveNodeReloadDelayMs,
```
--
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]
