[
https://issues.apache.org/jira/browse/SOLR-13439?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16845003#comment-16845003
]
Gus Heck commented on SOLR-13439:
---------------------------------
The timed watches suggestion has the following attributes that make me
nervous...
* Each watch implies an invocation of onStateChanged by the thread spawned for
each notification which is something of a waste, though that notification would
be a trivial {{return false}}, that still gets evaluated by the for loop over
all watches.
* From a usability and api design standpoint I really don't like setting a
watch you plan to ignore just for the caching side effect of the watch's
existence. It's just not straightforward. If you want to get notified when
something happens, set a watch, if you want caching the api should be clear
that that's what you're requesting (clarity improvements below).
* It's unclear to me how I avoid adding a watch on every request if I want to
check a collection property during a request. I don't know a way of
distinguishing my watch from a watch set by someone else (who might care about
the notification via onStateChanged), and if I add/remove one every request
it's not clear I'm getting caching between requests (even if it did somehow
retain the caching, that's very counter intuitive for users of the API?)
* Even if I simply trust in the side effects due to the existence of someone
else's watch it then becomes possible (via that watch returning true from
onStateChanged) for the cache to disappear out from under me and suddenly calls
to getCollectionProperties() are all expensive again.
How about this counter proposal:
* The existing getCollectionProperties() method remain a simple single shot
call that will pull from the cache or get from zk as it does now and not effect
caching in any way.
* My logic for expiration based caching be moved to a new method
getAndCacheCollectionProperties()
* The anonymous wrapper in {{wrapWatcher(final Watcher watcher)}} be converted
to a real class that performs a pass through in equals/hashcode for the
underlying watcher instance, thus allowing code that sends in the same watch
object to have the zkClient function as intended and not accumulate duplicates
that differ only by their anonymous wrapper.
* Retain a set of PropsWatcher objects such that we can re-use them and thus
avoid creating duplicate watches. These objects are very lightweight, but if we
want to worry about long running systems accumulating these objects as
collections come and go from the node it can also be an expiring cache with a
much longer time scale (days), and we accept the duplication of watches on a
time scale of days as inconsequential.
* When SOLR-8346 becomes available, add a cache expiration callback to
leverage ZOOKEEPER-442.
The fact that caching and watches for properties are tied to each other in a
way that is visible to callers of the API seems unnecessarily complicated, but
probably can't be fixed for backward compatibility reasons. If I could start
from scratch, watching would only provide notification of changes and have no
visible side effects, and caching would be an optimization option available for
fetching, not related to watches. Under the covers they might cooperate to
avoid multiple watches, but that should be undetectable to the user of the API.
> 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
