patsonluk commented on code in PR #909: URL: https://github.com/apache/solr/pull/909#discussion_r921486062
########## solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java: ########## @@ -246,6 +245,138 @@ 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 { + private final ConcurrentHashMap<String, StatefulCollectionWatch> + statefulWatchesByCollectionName = new ConcurrentHashMap<>(); + + /** + * 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 watch = + statefulWatchesByCollectionName.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 statefulWatchesByCollectionName.entrySet().stream() + .filter( + (Entry<String, StatefulCollectionWatch> entry) -> + entry.getValue().currentState != null) + .map(Entry::getKey) + .collect(Collectors.toUnmodifiableSet()); + } + + /** + * Gets the count of active collections (collections that exist) being watched + * + * @return the count of active collections + */ + private long activeCollectionCount() { + return statefulWatchesByCollectionName.entrySet().stream() + .filter( + (Entry<String, StatefulCollectionWatch> entry) -> + entry.getValue().currentState != null) + .count(); + } + + private Set<String> watchedCollections() { + return Collections.unmodifiableSet(statefulWatchesByCollectionName.keySet()); + } + + private Set<Entry<String, StatefulCollectionWatch>> + watchedCollectionEntries() { + return Collections.unmodifiableSet(statefulWatchesByCollectionName.entrySet()); + } + + /** + * 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 + */ + private boolean updateDocCollection(String collection, DocCollection newState) { + StatefulCollectionWatch finalWatch = + statefulWatchesByCollectionName.computeIfPresent( + collection, + (col, watch) -> { + DocCollection oldState = watch.currentState; + if (oldState == null && newState == null) { + // OK, the collection not yet exist in ZK or already deleted + } else if (oldState == null) { + if (log.isDebugEnabled()) { + log.debug("Add data for [{}] ver [{}]", collection, newState.getZNodeVersion()); + } + watch.currentState = newState; + } else if (newState == null) { + log.debug("Removing cached collection state for [{}]", collection); + watch.currentState = null; + } else { // both new and old states are non-null + int oldCVersion = + oldState.getPerReplicaStates() == null + ? -1 + : oldState.getPerReplicaStates().cversion; + int newCVersion = + newState.getPerReplicaStates() == null + ? -1 + : newState.getPerReplicaStates().cversion; + if (oldState.getZNodeVersion() < newState.getZNodeVersion() + || oldCVersion < newCVersion) { + watch.currentState = newState; + if (log.isDebugEnabled()) { + log.debug( + "Updating data for [{}] from [{}] to [{}]", + collection, + oldState.getZNodeVersion(), + newState.getZNodeVersion()); + } + } + } + return watch; + }); + return finalWatch != null; + } Review Comment: @HoustonPutman Your implementation makes perfect sense as it does reflect whether the state has been updated, which if the existing [`updateWatchedCollection` before our change](https://github.com/apache/solr/blob/7339c37ad5893ad003e3d73fa72007e1ddd60747/solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java#L1951) does behave as described in method comment, then it will be all good! Unfortunately, the original `updateWatchedCollection` basically has 2 cases which violate what it describes as `returns true if the state has changed`: 1. If both new and old states are null (which could happen if either the collection is not yet created or is removed), it would still return `true` 2. If the old state has version number >= the new state, even though there's no update, it would still return `true` The 2nd case in particular could be problematic, as described in the comment: ``` // no change to state, but we might have been triggered by the addition of a // state watcher, so run notifications ``` Basically the method is trying to behave differently because it has knowledge of certain behavior requirement of the caller, such reverse dependency is usually not desirable. And my original attempt was to remove such reverse dependency by simply observing what does the return value actually mean after I simplified the logic to mimic the exact return value of the original `updateWatchedCollection`. My proposed change turns out to be equally confusing 😓 -- 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