patsonluk commented on code in PR #909:
URL: https://github.com/apache/solr/pull/909#discussion_r911530388
##########
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -326,12 +408,11 @@ public void forciblyRefreshAllClusterStateSlow() throws
KeeperException, Interru
// No need to set watchers because we should already have watchers
registered for everything.
refreshCollectionList(null);
refreshLiveNodes(null);
- // Need a copy so we don't delete from what we're iterating over.
- Collection<String> safeCopy = new
ArrayList<>(watchedCollectionStates.keySet());
+
Set<String> updatedCollections = new HashSet<>();
- for (String coll : safeCopy) {
+ for (String coll : collectionWatches.keySet()) {
Review Comment:
Ah yes. This is one of the change that Im not 100% sure as it changes the
behavior. It was buried in one of the original questions:
1. In `forciblyRefreshAllClusterStateSlow`, which we are proposing to just
replace `watchedCollectionStates.keySet()` with `collectionWatches.keySet()`,
the reasoning is that we should forcibly update every collection that is
registered in `collectionWatches`, even if previous fetch on such collection
returned null (ie `watchedCollectionStates` would have no entry on it). This is
a minor change of behavior but I think it is more "correct"? Would really need
more experienced Solr dev to share their thoughts here 🙏🏼
So my reasoning is that `forciblyRefreshAllClusterStateSlow` is supposed to
"discover" new collections if it was previously watched but not yet existed in
last fetch.
--
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]
