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

Thanks [~prusko], this looks very good. Some comments:
  public Map<String,String> getCollectionProperties(final String collection) {
    Map<String,String> properties = watchedCollectionProps.get(collection);
    if (properties == null) {
      try {
        properties = fetchCollectionProperties(collection, null);
      } catch (Exception e) {
        throw new SolrException(ErrorCode.SERVER_ERROR, "Error reading 
collection properties", e);

    return properties;
I believe you are intentionally not adding the fetched properties back to the 
{{watchedCollectionProps}} (because there is no watch, they would get stale). 
Could you add a comment about it? I spent some time trying to figure out if 
that was intentional or no. Also, could you add javadocs to this method?
 In {{PropsWatcher.refreshAndWatch()}} you have this code:
} catch (KeeperException e) {
        LOG.error("Unwatched collection: [{}]", coll, e);
What do you mean there with “Unwatched collection”?

If I understand correctly, on the first time someone calls 
{{registerCollectionPropsWatcher}} for a particular zkStateReader and 
collection, the watcher will be executed, but from then on, that’s not going to 
happen again. Is this correct? If so, I believe we should make that consistent. 
Components don’t necessarily know in which order they are loaded and this may 
generate some strange behavior, also, multiple cores could belong to the same 
collection in the same node and only the first one will trigger the watcher on 
the registration. Maybe {{PropsWatcher.refreshAndWait()}} should receive a 
boolean parameter and use that to know if it has to notify or not the watchers. 
Then, when you are doing {{new PropsWatcher(collection).refreshAndWatch();}} 
you’d use {{false}}, and {{true}} from {{PropsWatcher.process(…)}}. WDYT?

In {{refreshAndWatch()}} you are synchronizing on {{getUpdateLock()}}, which 
sounds like overkill. I’m wondering if we really need any synchronization here 
since we are just submitting tasks to a multi-thread Executor.

Should {{removeCollectionPropsWatcher}} also remove the collection from 
{{collectionPropsWatches}} in case of {{canBeRemoved()==true}}?

Could you add a test that validates that collection properties are correctly 
backed up and restored when using BACKUP and RESTORE APIs?

> Add collection level properties
> -------------------------------
>                 Key: SOLR-11960
>                 URL: https://issues.apache.org/jira/browse/SOLR-11960
>             Project: Solr
>          Issue Type: New Feature
>      Security Level: Public(Default Security Level. Issues are Public) 
>            Reporter: Peter Rusko
>            Priority: Major
>         Attachments: 0001-SOLR-11960-add-collection-properties-support.patch, 
> SOLR-11960.patch
> Solr has cluster properties, but no easy and extendable way of defining 
> properties that affect a single collection. Collection properties could be 
> stored in a single zookeeper node per collection, making it possible to 
> trigger zookeeper watchers for only those Solr nodes that have cores of that 
> collection.

This message was sent by Atlassian JIRA

To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org

Reply via email to