[jira] [Commented] (SOLR-13439) Make collection properties easier and safer to use in code

["Tomás Fernández Löbbe (JIRA)" <ji...@apache.org>]

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

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

I don’t really like the idea of the time-based cache expire. In my mind, it’s better if whatever component sets up the watcher is responsible of removing it instead of adding a dependency on time. I won’t oppose if you still want to go this route.

Some comments on the patch:
{noformat}
-  private final ConcurrentHashMap<String, VersionedCollectionProps> watchedCollectionProps = new ConcurrentHashMap<>();
+  private final ConditionalExpiringCache<VersionedCollectionProps> watchedCollectionProps =
+      new ConditionalExpiringCache<>(new Predicate<Entry<String, VersionedCollectionProps>>() {
+        @Override
+        public boolean test(Entry<String, VersionedCollectionProps> entry) {
+          return collectionWatches.containsKey(entry.getKey());
+        }
+      }, CACHE_COLLECTION_PROPS_REAPER_INTERVAL, CACHE_COLLECTION_PROPS_FOR_NANOS);
{noformat}
By using only {{collectionWatches}}, the issue I mentioned in SOLR-13420 will happen. If something is watching collection properties without watching the collection, the cache will expire the key and, and the next time the watch fires it will be dropped.

{noformat}
+            properties = fetchCollectionProperties(collection, null).props;
+            // regularly requested in updates so start caching, note that this method registers it's parent object
+            // automatically.
+            new PropsWatcher(collection).refreshAndWatch(false);
+          }
{noformat}
{{new PropsWatcher(collection).refreshAndWatch(false)}} should set the properties, you seem to be fetching twice here.

It would be nice to have tests for {{ConditionalExpiringCache}}

> 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
>
>
> (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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org