[ 
https://issues.apache.org/jira/browse/SOLR-13418?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16827091#comment-16827091
 ] 

Tomás Fernández Löbbe commented on SOLR-13418:
----------------------------------------------

bq. Thus since it will always be needed on every update request adding a 
watcher as you suggest and caching locally would merely duplicate what's 
already cached at the ZkStateReader level
True. As long as you have at least one watcher set on the collection, doing a 
call to {{getCollectionProperties}} should just respond from memory, so no need 
to duplicate that.

bq. and delay the uptake of new information by the time it took the 
notification thread to make the adjustment.
This is just the time for the ZooKeeper watch to be processed. I think this is 
an OK delay, since it only applies when a collection property is being updated 
(if the update arrives a couple of milliseconds later, it shouldn’t be such an 
impact in practice, in your test, you should setup some mechanism to wait for 
the change though but that is anyway the case unless you fetch from zk every 
time)

bq. I guess it's true that if one is using a non-java client or otherwise not 
using CloudSolrClient, this would lead to caching collection properties 
information on nodes not containing that collection but that would be necessary 
anyway to ensure proper handling of RoutedAliases.
If the collection is not present in the node, Solr wouldn’t create the URP to 
handle the update, I don’t think in this case you are leaking properties, 
right? The leak comes in the case of collections getting moved out of the node 
or with collections being created/deleted. It’s not only a memory leak, but 
also a ZooKeeper watch leak.

bq. I am heartened by your characterization of the configuration stored as 
"simple"
I meant this as compare with the config API, that allows you to do much bigger 
changes, like add/remove components, but for which there is no notification 
mechanism, and relies on core reloads (in addition that the target is really 
the configset, not the collection). 

Why is TimeRoutedAliasUpdateProcessor created out of a static wrap function in 
DUP instead of a Factory?

{code}
-      if (!collectionPropsWatches.containsKey(coll)) {
+      if (!watchedCollectionProps.containsKey(coll)) {
         // No one can be notified of the change, we can ignore it and "unset" 
the watch
         log.debug("Ignoring property change for collection {}", coll);
         return;
{code}
With this change, if the collection is not being watched, a collection property 
watcher would not be notified. Also, since the {{collectionPropsWatches}} would 
still contain the collection name, the value of the properties in the map would 
go stale. What’s the intention of this change?


> ZkStateReader.PropsWatcher synchronizes on a string value & doesn't track zk 
> version
> ------------------------------------------------------------------------------------
>
>                 Key: SOLR-13418
>                 URL: https://issues.apache.org/jira/browse/SOLR-13418
>             Project: Solr
>          Issue Type: Bug
>      Security Level: Public(Default Security Level. Issues are Public) 
>          Components: SolrCloud
>    Affects Versions: 8.0, master (9.0)
>            Reporter: Gus Heck
>            Assignee: Gus Heck
>            Priority: Major
>             Fix For: 8.1, master (9.0)
>
>
> While contemplating adding better caching to collection properties to avoid 
> repeated calls to ZK from code that wishes to consult collection properties, 
> I noticed that the existing PropsWatcher class is synchronizing on a string 
> value for the name of a collection. Synchronizing on strings is bad practice, 
> given that you never know if the string might have been interned, and 
> therefore someone else might also synchronizing on the same object without 
> your knowledge creating contention or even deadlocks. Also this code doesn't 
> seem to be doing anything to check ZK version information, so it seems 
> possible that out of order processing by threads could wind up caching out of 
> date data. 



--
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

Reply via email to