patsonluk commented on code in PR #909:
URL: https://github.com/apache/solr/pull/909#discussion_r915129741
##########
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -246,6 +244,95 @@ public boolean canBeRemoved() {
}
}
+ /**
+ * A ConcurrentHashMap of active watcher by collection name
+ *
+ * <p>Each watcher DocCollectionWatch also contains the latest DocCollection
(state) observed
+ */
+ private static class DocCollectionWatches
+ extends ConcurrentHashMap<String,
StatefulCollectionWatch<DocCollectionWatcher>> {
+
+ /**
+ * Gets the DocCollection (state) of the collection which the
corresponding watch last observed
+ *
+ * @param collection the collection name to get DocCollection on
+ * @return The last observed DocCollection(state). if null, that means
there's no such
+ * collection.
+ */
+ private DocCollection getDocCollection(String collection) {
+ StatefulCollectionWatch<DocCollectionWatcher> watch = get(collection);
+ return watch != null ? watch.currentState : null;
+ }
+
+ /**
+ * Gets the active collections (collections that exist) being watched
+ *
+ * @return an immutable set of active collection names
+ */
+ private Set<String> activeCollections() {
+ return this.entrySet().stream()
+ .filter(
+ (Entry<String, StatefulCollectionWatch<DocCollectionWatcher>>
entry) ->
+ entry.getValue().currentState != null)
+ .map(Entry::getKey)
+ .collect(Collectors.toUnmodifiableSet());
+ }
+
+ /**
+ * Updates the latest observed DocCollection (state) of the {@link
StatefulCollectionWatch} if
+ * the collection is being watched
+ *
+ * @param collection the collection name
+ * @param newState the new DocCollection (state) observed
+ * @return whether an active watch exists for such collection
Review Comment:
Good question @HoustonPutman ! I remember trying to make my method to behave
the same as `updateWatchedCollection`, that it returns `true` even if:
1. Both oldState and newState are null, this could happen after deletion
when i debug
2. newState has older version than oldState, hence no updates are made
After I simplified my logic, i notice that the end result turns out to be
just "whether the collection was watched" , hence i changed the javadoc to
describe this exact behavior 😓
By looking at the existing `updateWatchedCollection`, it appears to only
return false if the collection is no longer watched right at the beginning of
the method body with exception that new state is null? (and the new class
should avoid the case which `collectionWatches` changed during the call)
Though I do share similar concerns that some places that rely on the return
values of the existing method, might now seem a bit odd ie:
```
if (collectionWatches.updateDocCollection(coll, newState)) {
updatedCollections.add(coll);
```
I am not sure if I completely follow the original comments in the code
```
// no change to state, but we might have been triggered by the addition of a
// state watcher, so run notifications
```
So is the return value actually flags whether we should run notifications (i
assume it means running `constructState`?). If so, would it make sense to just
call this flag `shouldReconstructState` or something? 🤔
--
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
