[
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:19 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 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 ?
was (Author: noble.paul):
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: [email protected]
For additional commands, e-mail: [email protected]
