[
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
