[ https://issues.apache.org/jira/browse/SOLR-5473?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13979259#comment-13979259 ]
Noble Paul edited comment on SOLR-5473 at 4/24/14 4:45 AM: ----------------------------------------------------------- Before I comment further, 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 convention all the serialization/deserialization libraries follow 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 ? was (Author: noble.paul): Before I comment further, 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