HoustonPutman commented on code in PR #909:
URL: https://github.com/apache/solr/pull/909#discussion_r921503461
##########
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:
> 2. If the old state has version number >= the new state, even though
there's no update, it would still return true
As I mentioned in
https://github.com/apache/solr/pull/909#discussion_r921419678, I don't think
that comment is correct, so we should be good to go with the new behavior.
> 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
It's confusing but I don't think that is the case. Looking at the relevant
part of the old implementation:
```java
if (oldState == null) {
if (watchedCollectionStates.putIfAbsent(coll, newState) == null) {
if (log.isDebugEnabled()) {
log.debug("Add data for [{}] ver [{}]", coll,
newState.getZNodeVersion());
}
updated = true;
break;
}
}
```
[putIfAbsent()](https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#putIfAbsent-K-V-)
returns the previous value associated with that key, so `null` means that
there was no previous value and the `putIfAbsent()` successfully executed. It
doesn't actually check what the `newState` is.
So it would return `true` if the old state was null, no matter what. We
probably don't want to keep that functionality. No need to update collections
if a state went from `null` -> `null`.
--
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
