[ https://issues.apache.org/jira/browse/SOLR-13439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16840611#comment-16840611 ]
Gus Heck commented on SOLR-13439: --------------------------------- {quote}Exactly, which is why the option of not setting a watch exists for cases that only need infrequent access. {quote} If that option is truly important, we can have 2 methods, one that does cause a watch/cache and one that hits zookeeper every time, getCollectionProperties() and getCachedCollectionProperties() perhaps? {quote}Not on only those cases. There are two more leak scenarios that I’d like to consider: # Even if the PropsWatcher expires and it’s removed from the cache, ZooKeeper server will still maintain the watch referenced to this zkClient, that means we just keep accumulating them for every call to getCollectionProperties. If you do this to many collections and collection properties changes are infrequent, this may become a problem. I believe the PropsWatcher object won’t be garbage collected, since it includes the callback.{quote} ZkStateReader lives at the CoreContainer level. so we have one per node, and thus one instance of the zk client. Looking into the zookeeper code, what I think I see is a map of path (String) to HashSet<Watcher>, but the watchers added to that set are instances of ServerCnxn.java (which implements watcher) which appears to also be the object representing the connection to the client, of which (I think) there will only be one per client. So the max load on the zookeeper server side for a given path is set by the number of Solr nodes observing that path. So the absolute worst case for the server in this code is {{collections X nodes}} if every collection has a shard on every node, the properties never change and every collection has it's properties queried. The worst case hasn't changed, though it does become more likely with the direction I'm heading. According to ZOOKEEPER-1177 a watch is about 100 bytes... so a 1000 collections on 100 nodes (100k watches) could generate up to 10Mb of watches on the server. According to that issue, performance of the "old" (which is what 3.4.14 would be since that issue is fixed for 3.6) is: {code:java} OLD [junit] 9736ms to add [junit] size:10000000 [junit] 0ms to size [junit] 5615ms to dumpwatches true [junit] 3035ms to dumpwatches false [junit] 5530ms to trigger{code} those numbers are for 10M watches so those operations on our 100K example is likely to run in the tens of ms. (the foregoing assuming I understood the code and the issue correctly, and a linear relationship vs number of watches). In zookeeper 3.5 we'll also have the opportunity to cull out watches on with ZOOKEEPER-442 So server side in zk, it looks like we should be able to handle some fairly beefy use cases, what's the max nodes times collections you think we need to support? Do you have large test clusters on which to validate these effects? (I do not unfortunately). As for GC I did think about that... references to a PropsWatcher are held in 2 places: # The ConditionalExpiringCache as a value # The closure created when the anonymous implementation of SolrZkWatcher is created Once the watch fires, the anonymous wrapper should not be referenced by zk code and become held by a lambda submitted to the executor, which in turn is discarded once the call to process() finishes. Once the watch has fired and the cache has expired there are no references held and it becomes GC eligible. GC is prevented however if the result of process() (called by the lambda) is to call refreshAndWatch() (in the event that a CollectionPropsWatch still exists) since that will put it back in the ConditionalExpiringCache with a renewed timestamp and re-register the watch for zk. {quote}2 Even if you keep calling getCollectionProperties on the same collection, but after expiration, you keep adding new watches, for the same path. That not only means leaks of watches in ZooKeeper server and PropsWatcher objects in Solr, but also that, if at some point later in time a collection property changes all those watches are going to fire at the same time. Even more, if at the time of the fire, the collection is in the cache (because of a recent call to getCollectionProperties), all those watches for the same path, will re-fetch the collections properties file and then re-set. This arguably can happen today, if someone uses watchers in the same way (add/remove watches rapidly), but the idea of the watch is to be longer lived (i.e. the life of the core), while with the patch it would happen with components that just call getCollectionProperties with some frequency (i.e. per request, in cases with low request rate) {quote} Some further research shows that we may have a subtle preexisting problem there. Zookeeper keeps a set of watches so adding the same watch object to it repeatedly is not a big deal at that level. However our code wraps every added watch in an anonymous instance meaning that you can add the exact same watch N times and your notification will get called N times. From zookeeper's perspective it's getting a new instance with a default implementation of .equals() every time. That feels like it might be an unintended effect or maybe even a bug. The wrapper seems to have been added as part of SOLR-6273 (CDCR) Maybe [~erickerickson] knows more? Also, my code shouldn't have been throwing away old watches, but saving them to be re-applied (or we could be carefully managing .equals() ) so that the Map<String, Set<Watcher>> in zookeeper can do it's job... that unassigned new instance creation made me nervous, but the prior code did it too and so I didn't stop to question it deeply. I'm still pondering your suggestion. More on that later. > Make collection properties easier and safer to use in code > ---------------------------------------------------------- > > Key: SOLR-13439 > URL: https://issues.apache.org/jira/browse/SOLR-13439 > Project: Solr > Issue Type: Improvement > Security Level: Public(Default Security Level. Issues are Public) > Components: SolrCloud > Affects Versions: master (9.0) > Reporter: Gus Heck > Assignee: Gus Heck > Priority: Major > Attachments: SOLR-13439.patch, SOLR-13439.patch > > > (breaking this out from SOLR-13420, please read there for further background) > Before this patch the api is quite confusing (IMHO): > # any code that wanted to know what the properties for a collection are > could call zkStateReader.getCollectionProperties(collection) but this was a > dangerous and trappy API because that was a query to zookeeper every time. If > a naive user auto-completed that in their IDE without investigating, heavy > use of zookeeper would ensue. > # To "do it right" for any code that might get called on a per-doc or per > request basis one had to cause caching by registering a watcher. At which > point the getCollectionProperties(collection) magically becomes safe to use, > but the watcher pattern probably looks famillar induces a user who hasn't > read the solr code closely to create their own cache and update it when their > watcher is notified. If the caching side effect of watches isn't understood > this will lead to many in-memory copies of collection properties maintained > in user code. > # This also creates a task to be scheduled on a thread (PropsNotification) > and induces an extra thread-scheduling lag before the changes can be observed > by user code. > # The code that cares about collection properties needs to have a lifecycle > tied to either a collection or something other object with an even more > ephemeral life cycle such as an URP. The user now also has to remember to > ensure the watch is unregistered, or there is a leak. > After this patch > # Calls to getCollectionProperties(collection) are always safe to use in any > code anywhere. Caching and cleanup are automatic. > # Code that really actually wants to know if a collection property changes > so it can wake up and do something (autoscaling?) still has the option of > registering a watcher that will asynchronously send them a notification. > # Updates can be observed sooner via getCollectionProperties with no need to > wait for a thread to run. (vs a cache held in user code) -- This message was sent by Atlassian JIRA (v7.6.3#76005) --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org