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

Noble Paul commented on SOLR-5473:
----------------------------------

Before I comment of the comments I would like to lay out the objective and 
design of this ticket. 

Splitting out the main clutserstate.json was not done just to make each state 
smaller, but also to make fewer nodes update the state changes. If you have a 
large no:of collections , most of the nodes will never need to know about most 
of the other collections. So , it is not a good idea to have an explosion on 
the no:of watchers because we split the monolithic clusterstate . I would say 
it actually would be bad for scalability. 

So the approach is , whenever a node has a core which is a member of a 
collection , register a watch for the state of that collection and when the 
last core that is a member of that collection is unloaded, remove the watch. 
For those who don't watch other collections they can still fetch the state 
directly on demand. 

The fact that the main clusterstate.json may not have all the collections means 
that it cannot hold all the states within it. Though the actual data inside the 
object does not change, the returned values can be different based on different 
situations. The system does not rely on the immutability of clusterstate 
anywhere, It just expects the method calls to provide the correct data all the 
time. In fact it is even plain wrong to hold on to the clusterstate object for 
later use because it could change the data at anytime. The only place where we 
reuse the object is inside Overseer where we know that the state is only 
modified by itself. ZkStateReader is the class that is responsible for keeping 
the states upto date and the best way to make the clusterstate APIs behave 
consistently is to have a reference of ZkStateReader


The latest patch is not a final one . The objective of that patch was to get 
the naming right . 

bq.Can you give a concrete example of how having that info(stateFormat) there 
will be useful? It still makes no sense to me.

 Though it is possible to deduce the format from where the data is fetched, It 
will have to be onus of the code that serialize/deserialize the object back and 
forth from json  to have format correctly. If it is stored within the object  
that will be be automatically be taken care of. If we introduce a new 
stateFormat=3 , the system will have already collections made with 
stateFormat=2 but that does not explicitly say so. So we will have to put some 
logic into the serialization/deserialization logic to differentiate 2 from 3 . 
So it is just safer and consistent to encode the version of the format with the 
payload itself. That is the way all the serialization/deserialization libraries 
work



getExternCollectionFresh, getCommonCollection etc are gone in the latest patch. 
The objective was to get rid of external/internal from the public APIs .The 
internal variables contain the external moniker for lack of better names . I 
hope it is not a problem. We can always add javadocs to make them clearer.


Actually I use the intellij project formatting after doing an 'ant idea' . I 
will have to investigate if/why it is different.


bq.Why do we have getStateVersion and stateFormat? Why > 1 and not =2?

If I introduce a new format 3, will it not include 2 also? That is just future 
proofing .
 

bq.This is not thread safe and ZkStateReader needs to be.
Also, -1 on this public variable on ZkStateReader. It's a bad design smell for 
good reason.

This has to be threadsafe. Will fix

I'll then have to make it a public setter method. Other than that there is no 
saner way for Overseer to expose the temporary data . I spent some time on 
figuring out a cleaner way , I will be glad to implement a better suggestion


bq.When you catch an InterruptedException you should do 
Thread.currenThread.interrupt() to reset the flag.

will do

bq.removeZkWatch() Should be marked internal then. Not a great design that has 
public internal methods on public objects though.

I would have preferred them to be package local, If possible. Unfortunately 
java does not allow it. What other choice do we have ?




> Make one state.json per collection
> ----------------------------------
>
>                 Key: SOLR-5473
>                 URL: https://issues.apache.org/jira/browse/SOLR-5473
>             Project: Solr
>          Issue Type: Sub-task
>          Components: SolrCloud
>            Reporter: Noble Paul
>            Assignee: Noble Paul
>             Fix For: 5.0
>
>         Attachments: SOLR-5473-74.patch, SOLR-5473-74.patch, 
> SOLR-5473-74.patch, SOLR-5473-74.patch, SOLR-5473-74.patch, 
> SOLR-5473-74.patch, SOLR-5473-74.patch, SOLR-5473-74.patch, 
> SOLR-5473-74.patch, SOLR-5473-74.patch, SOLR-5473-74.patch, 
> SOLR-5473-74.patch, SOLR-5473-74.patch, SOLR-5473-74.patch, 
> SOLR-5473-74.patch, SOLR-5473-74.patch, SOLR-5473-74.patch, 
> SOLR-5473-74.patch, SOLR-5473-74.patch, SOLR-5473-74.patch, 
> SOLR-5473-74.patch, SOLR-5473-74.patch, SOLR-5473-74.patch, 
> SOLR-5473-74.patch, SOLR-5473.patch, SOLR-5473.patch, SOLR-5473.patch, 
> SOLR-5473.patch, SOLR-5473.patch, SOLR-5473.patch, SOLR-5473.patch, 
> SOLR-5473.patch, SOLR-5473.patch, SOLR-5473.patch, SOLR-5473.patch, 
> SOLR-5473.patch, SOLR-5473.patch, SOLR-5473.patch, SOLR-5473.patch, 
> SOLR-5473.patch, SOLR-5473.patch, SOLR-5473.patch, ec2-23-20-119-52_solr.log, 
> ec2-50-16-38-73_solr.log
>
>
> As defined in the parent issue, store the states of each collection under 
> /collections/collectionname/state.json node



--
This message was sent by Atlassian JIRA
(v6.2#6252)

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

Reply via email to