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

Gus Heck commented on SOLR-11925:
---------------------------------

Took a look at the patch, here are my thoughts:

Like:
 * rename of handle response
 * fluent style for addMetadata (returning the command)
 * create and delete sections of results :)
 * Nice cleanup in invokeAction, especially collecting and propagating the 
exception earlier.

Question:
 * Why is remoteInvoke on RouedAliasCreateCollectionCommand when it's only used 
from TimeRoutedAliasUpdateProcessor?
 * Rename field on zkstateReader aliasesHolder to aliasesManager?
 * You added 2 parseSolrDateToInstant() methods with different params, but 
neither seems to be used?
 * in your cleanup of invokeAction etc, you seem to have dropped some 
conditionals looking for null or -1 error codes, which was added by MarkMiller 
in 2013... (bottom of handleResponse, which is now sentToOCPQueue) Any idea 
what that was trapping? Why is it not needed now?

Thought about:
 * Thinking about the validation in TimeRoutedAlias ... Does seem that if an 
alias metadata gets messed up causing validation issue, all operations sending 
data to that alias will start failing. This worried me initially, but is 
probably ok. Must not get used in ModifyAlias in the future, since that would 
make it impossible to repair broken metadata. Not something we are doing now, 
but a potential pitfall. In constructor validation of values often worries me 
due to the inability to model busted state if state becomes busted.
 * We seem to rely on applyModificationandExportToZk to ensure that the alias 
is updated before deleting collections, but I think that's a little risky given 
the need for zookeeper watches to update. If we delete collections quickly 
after that nodes that have not processed watch notifications may send to the 
deleted shard? Do we need another (ugly, but magic) 100ms wait
 * I think we can worry about making deletion async if that becomes a pain 
point later. Simple to start as you have it now is good.

Suggest:
 * Javadoc for deleteCollectionsAndUpdateAlias() - especially explanation of 
the correct notion for the "from" parameter.
 * Similarly doc for autoDeleteBeforeTime property on TimeRoutedAlias... time 
stuff is always fiddly, helps to have some doc. Not sure why it's not 
autoDeleteAfterInterval? Maybe you mean autoDeleteIfBefore?

> Auto delete oldest collections in a time routed alias
> -----------------------------------------------------
>
>                 Key: SOLR-11925
>                 URL: https://issues.apache.org/jira/browse/SOLR-11925
>             Project: Solr
>          Issue Type: Sub-task
>      Security Level: Public(Default Security Level. Issues are Public) 
>          Components: SolrCloud
>            Reporter: David Smiley
>            Assignee: David Smiley
>            Priority: Major
>             Fix For: 7.3
>
>         Attachments: SOLR-11925.patch
>
>
> The oldest collections in a Time Routed Alias should be automatically 
> deleted, according to a new alias metadata that establishes how long.  It can 
> be checked as new data flows in at TimeRoutedAliasUpdateProcessor and thus it 
> won't occur if new data isn't coming in.



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