madrob commented on code in PR #909:
URL: https://github.com/apache/solr/pull/909#discussion_r915147046
##########
solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java:
##########
@@ -246,6 +244,90 @@ 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,
DocCollectionWatch<DocCollectionWatcher>> {
Review Comment:
You covered it pretty nicely, Houston.
The main concern is really about future maintainability. Right now we're
handling it fine and the boundaries are clear and we're not doing anything that
we're not supposed to be doing. But the risk is that at some point this class
ends up getting treated like any other Map (if it gets passed or returned
somewhere) and people start calling map methods on it. Then any additional
internal state that we're keeping (which right now it doesn't look like there
is any) can get messed up. For example, if we had a variable tracking the
number of watches fired and somebody else added a new entry to the map without
using our abstractions then our count would be off. So we want to expose the
things meant to be exposed and encapsulate the rest.
Hopefully that is clear enough, it's a lot of text and I'm not sure if I
made it worse.
--
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]
