You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@helix.apache.org by Zhen Zhang <ne...@gmail.com> on 2013/08/23 02:02:30 UTC
Review Request 13754: [HELIX-109] Review Helix model package
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13754/
-----------------------------------------------------------
Review request for helix, Kanak Biscuitwala, Kishore Gopalakrishna, and Shi Lu.
Repository: helix-git
Description
-------
Initial change for refactor helix model. Roughly follow the design at:
https://cwiki.apache.org/confluence/display/HELIX/API+Redesign
An example is shown in ClusterReader where using a Cluster instance, we can retrieve any information needed for the rebalancer. The change is incomplete.
Diffs
-----
helix-core/src/main/java/org/apache/helix/api/Cluster.java e69de29
helix-core/src/main/java/org/apache/helix/api/ClusterReader.java e69de29
helix-core/src/main/java/org/apache/helix/api/Controller.java e69de29
helix-core/src/main/java/org/apache/helix/api/CurState.java e69de29
helix-core/src/main/java/org/apache/helix/api/ExtView.java e69de29
helix-core/src/main/java/org/apache/helix/api/HelixVersion.java e69de29
helix-core/src/main/java/org/apache/helix/api/LeaderController.java e69de29
helix-core/src/main/java/org/apache/helix/api/LiveParticipant.java e69de29
helix-core/src/main/java/org/apache/helix/api/Model.java e69de29
helix-core/src/main/java/org/apache/helix/api/Msg.java e69de29
helix-core/src/main/java/org/apache/helix/api/Participant.java e69de29
helix-core/src/main/java/org/apache/helix/api/Partition.java e69de29
helix-core/src/main/java/org/apache/helix/api/Resource.java e69de29
helix-core/src/main/java/org/apache/helix/api/RunningInstance.java e69de29
helix-core/src/main/java/org/apache/helix/api/Spectator.java e69de29
helix-core/src/main/java/org/apache/helix/api/StateModelDef.java e69de29
Diff: https://reviews.apache.org/r/13754/diff/
Testing
-------
Thanks,
Zhen Zhang
Re: Review Request 13754: [HELIX-109] Review Helix model package
Posted by Kanak Biscuitwala <ka...@hotmail.com>.
> On Aug. 23, 2013, 12:27 a.m., Kanak Biscuitwala wrote:
> > Below are a bunch of thoughts on the design. We should discuss them offline as well. Pasting suggestions from Kishore below for further discussion:
> >
> > - CLUSTER
> > - PARTICIPANT(*)
> > - CONFIG
> > - SPECTATOR(*)
> > - CONTROLLER(*)
> > - RESOURCE
> > - CONFIG
> > - IDEALSTATE
> > - EXTERNALVIEW
> > - CURRENTSTATE
> >
> > State machine and constraints can be a separate dimension.
Also, I don't currently see a way to get the current ideal state of a resource. Is this by design?
- Kanak
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13754/#review25443
-----------------------------------------------------------
On Aug. 23, 2013, 12:02 a.m., Zhen Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13754/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2013, 12:02 a.m.)
>
>
> Review request for helix, Kanak Biscuitwala, Kishore Gopalakrishna, and Shi Lu.
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> Initial change for refactor helix model. Roughly follow the design at:
> https://cwiki.apache.org/confluence/display/HELIX/API+Redesign
>
> An example is shown in ClusterReader where using a Cluster instance, we can retrieve any information needed for the rebalancer. The change is incomplete.
>
>
> Diffs
> -----
>
> helix-core/src/main/java/org/apache/helix/api/Cluster.java e69de29
> helix-core/src/main/java/org/apache/helix/api/ClusterReader.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Controller.java e69de29
> helix-core/src/main/java/org/apache/helix/api/CurState.java e69de29
> helix-core/src/main/java/org/apache/helix/api/ExtView.java e69de29
> helix-core/src/main/java/org/apache/helix/api/HelixVersion.java e69de29
> helix-core/src/main/java/org/apache/helix/api/LeaderController.java e69de29
> helix-core/src/main/java/org/apache/helix/api/LiveParticipant.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Model.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Msg.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Participant.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Partition.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Resource.java e69de29
> helix-core/src/main/java/org/apache/helix/api/RunningInstance.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Spectator.java e69de29
> helix-core/src/main/java/org/apache/helix/api/StateModelDef.java e69de29
>
> Diff: https://reviews.apache.org/r/13754/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhen Zhang
>
>
Re: Review Request 13754: [HELIX-109] Review Helix model package
Posted by Kanak Biscuitwala <ka...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13754/#review25443
-----------------------------------------------------------
Below are a bunch of thoughts on the design. We should discuss them offline as well. Pasting suggestions from Kishore below for further discussion:
- CLUSTER
- PARTICIPANT(*)
- CONFIG
- SPECTATOR(*)
- CONTROLLER(*)
- RESOURCE
- CONFIG
- IDEALSTATE
- EXTERNALVIEW
- CURRENTSTATE
State machine and constraints can be a separate dimension.
helix-core/src/main/java/org/apache/helix/api/Cluster.java
<https://reviews.apache.org/r/13754/#comment49845>
Specify what maps to what. In this case I guess it's name to object.
helix-core/src/main/java/org/apache/helix/api/ClusterReader.java
<https://reviews.apache.org/r/13754/#comment49844>
Maybe add some basic sanity checks as unit tests. I can help with that too.
helix-core/src/main/java/org/apache/helix/api/ExtView.java
<https://reviews.apache.org/r/13754/#comment49846>
A thought would be to make ExternalView a collection of CurrentStates. On the other hand, maybe this is also OK because it matches the IdealState interface.
helix-core/src/main/java/org/apache/helix/api/LeaderController.java
<https://reviews.apache.org/r/13754/#comment49843>
Please use the code format template.
helix-core/src/main/java/org/apache/helix/api/LiveParticipant.java
<https://reviews.apache.org/r/13754/#comment49847>
Is a separate object for live participants necessary? It could be a method in participant called isLive and then cluster could implement getLiveParticipants and filter internally
helix-core/src/main/java/org/apache/helix/api/Model.java
<https://reviews.apache.org/r/13754/#comment49842>
Is there a particular reason every class should derive from Model? It might confuse users. Does it buy us any flexibility?
helix-core/src/main/java/org/apache/helix/api/Msg.java
<https://reviews.apache.org/r/13754/#comment49848>
Are messages necessary to expose in this API?
helix-core/src/main/java/org/apache/helix/api/Spectator.java
<https://reviews.apache.org/r/13754/#comment49849>
should this extend RunningInstance?
helix-core/src/main/java/org/apache/helix/api/StateModelDef.java
<https://reviews.apache.org/r/13754/#comment49850>
what about transitions?
- Kanak Biscuitwala
On Aug. 23, 2013, 12:02 a.m., Zhen Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13754/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2013, 12:02 a.m.)
>
>
> Review request for helix, Kanak Biscuitwala, Kishore Gopalakrishna, and Shi Lu.
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> Initial change for refactor helix model. Roughly follow the design at:
> https://cwiki.apache.org/confluence/display/HELIX/API+Redesign
>
> An example is shown in ClusterReader where using a Cluster instance, we can retrieve any information needed for the rebalancer. The change is incomplete.
>
>
> Diffs
> -----
>
> helix-core/src/main/java/org/apache/helix/api/Cluster.java e69de29
> helix-core/src/main/java/org/apache/helix/api/ClusterReader.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Controller.java e69de29
> helix-core/src/main/java/org/apache/helix/api/CurState.java e69de29
> helix-core/src/main/java/org/apache/helix/api/ExtView.java e69de29
> helix-core/src/main/java/org/apache/helix/api/HelixVersion.java e69de29
> helix-core/src/main/java/org/apache/helix/api/LeaderController.java e69de29
> helix-core/src/main/java/org/apache/helix/api/LiveParticipant.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Model.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Msg.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Participant.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Partition.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Resource.java e69de29
> helix-core/src/main/java/org/apache/helix/api/RunningInstance.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Spectator.java e69de29
> helix-core/src/main/java/org/apache/helix/api/StateModelDef.java e69de29
>
> Diff: https://reviews.apache.org/r/13754/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhen Zhang
>
>
Re: Review Request 13754: [HELIX-109] Review Helix model package
Posted by Kanak Biscuitwala <ka...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13754/#review25493
-----------------------------------------------------------
Ship it!
I think this is a good start.
helix-core/src/main/java/org/apache/helix/api/Participant.java
<https://reviews.apache.org/r/13754/#comment49943>
May be nice to note for this and other functions that the returned collection is immutable.
- Kanak Biscuitwala
On Aug. 24, 2013, 1:33 a.m., Zhen Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13754/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2013, 1:33 a.m.)
>
>
> Review request for helix, Kanak Biscuitwala, Kishore Gopalakrishna, and Shi Lu.
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> Initial change for refactor helix model. Roughly follow the design at:
> https://cwiki.apache.org/confluence/display/HELIX/API+Redesign
>
> An example is shown in ClusterReader where using a Cluster instance, we can retrieve any information needed for the rebalancer. The change is incomplete.
>
>
> Diffs
> -----
>
> helix-core/src/main/java/org/apache/helix/api/Cluster.java e69de29
> helix-core/src/main/java/org/apache/helix/api/ClusterReader.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Controller.java e69de29
> helix-core/src/main/java/org/apache/helix/api/CurState.java e69de29
> helix-core/src/main/java/org/apache/helix/api/ExtView.java e69de29
> helix-core/src/main/java/org/apache/helix/api/HelixVersion.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Msg.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Participant.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Partition.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Resource.java e69de29
> helix-core/src/main/java/org/apache/helix/api/RunningInstance.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Spectator.java e69de29
>
> Diff: https://reviews.apache.org/r/13754/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhen Zhang
>
>
Re: Review Request 13754: [HELIX-109] Review Helix model package
Posted by Zhen Zhang <ne...@gmail.com>.
> On Aug. 24, 2013, 4:35 a.m., Kishore Gopalakrishna wrote:
> > can we have separate packages for model, role, listeners ?
> >
> > Lets avoid having strings, even things like cluster name, resource name, partition name should have concrete classes. Also having builder for every object is helpful. what do you think
>
> Kanak Biscuitwala wrote:
> - Package separation makes sense. Currently, there's only model and role classes. I think a new discussion is necessary to decide how to do listeners right (and more broadly, concrete use cases for this new API).
>
> - I agree with the string comment. Many (if not all) of the Map<String, Obj> could be List/Set<Obj> instead and others can be Map<Obj1, Obj2>.
>
> - I think the following can be candidates for builders: Cluster, Resource, Participant, and maybe CurState and ExtView. I can create a separate Jira and work on those.
>
> I'm sure Jason has additional thoughts.
Some comments about the 2nd point: using Map instead of List/Set might be convenient since in many cases, we need to index ideal-state/current-state/message by id's. Agree to use Id instead of String, but it will be just a wrapper around string and we need to override equal() and hash() methods. To avoid code duplication, we may have an Id base class and ResourceId, ClusterId, ParticipantId, PartitionId, etc will be empty derived classes from Id class?
- Zhen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13754/#review25495
-----------------------------------------------------------
On Aug. 24, 2013, 1:33 a.m., Zhen Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13754/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2013, 1:33 a.m.)
>
>
> Review request for helix, Kanak Biscuitwala, Kishore Gopalakrishna, and Shi Lu.
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> Initial change for refactor helix model. Roughly follow the design at:
> https://cwiki.apache.org/confluence/display/HELIX/API+Redesign
>
> An example is shown in ClusterReader where using a Cluster instance, we can retrieve any information needed for the rebalancer. The change is incomplete.
>
>
> Diffs
> -----
>
> helix-core/src/main/java/org/apache/helix/api/Cluster.java e69de29
> helix-core/src/main/java/org/apache/helix/api/ClusterReader.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Controller.java e69de29
> helix-core/src/main/java/org/apache/helix/api/CurState.java e69de29
> helix-core/src/main/java/org/apache/helix/api/ExtView.java e69de29
> helix-core/src/main/java/org/apache/helix/api/HelixVersion.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Msg.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Participant.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Partition.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Resource.java e69de29
> helix-core/src/main/java/org/apache/helix/api/RunningInstance.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Spectator.java e69de29
>
> Diff: https://reviews.apache.org/r/13754/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhen Zhang
>
>
Re: Review Request 13754: [HELIX-109] Review Helix model package
Posted by Kanak Biscuitwala <ka...@hotmail.com>.
> On Aug. 24, 2013, 4:35 a.m., Kishore Gopalakrishna wrote:
> > can we have separate packages for model, role, listeners ?
> >
> > Lets avoid having strings, even things like cluster name, resource name, partition name should have concrete classes. Also having builder for every object is helpful. what do you think
- Package separation makes sense. Currently, there's only model and role classes. I think a new discussion is necessary to decide how to do listeners right (and more broadly, concrete use cases for this new API).
- I agree with the string comment. Many (if not all) of the Map<String, Obj> could be List/Set<Obj> instead and others can be Map<Obj1, Obj2>.
- I think the following can be candidates for builders: Cluster, Resource, Participant, and maybe CurState and ExtView. I can create a separate Jira and work on those.
I'm sure Jason has additional thoughts.
- Kanak
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13754/#review25495
-----------------------------------------------------------
On Aug. 24, 2013, 1:33 a.m., Zhen Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13754/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2013, 1:33 a.m.)
>
>
> Review request for helix, Kanak Biscuitwala, Kishore Gopalakrishna, and Shi Lu.
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> Initial change for refactor helix model. Roughly follow the design at:
> https://cwiki.apache.org/confluence/display/HELIX/API+Redesign
>
> An example is shown in ClusterReader where using a Cluster instance, we can retrieve any information needed for the rebalancer. The change is incomplete.
>
>
> Diffs
> -----
>
> helix-core/src/main/java/org/apache/helix/api/Cluster.java e69de29
> helix-core/src/main/java/org/apache/helix/api/ClusterReader.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Controller.java e69de29
> helix-core/src/main/java/org/apache/helix/api/CurState.java e69de29
> helix-core/src/main/java/org/apache/helix/api/ExtView.java e69de29
> helix-core/src/main/java/org/apache/helix/api/HelixVersion.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Msg.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Participant.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Partition.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Resource.java e69de29
> helix-core/src/main/java/org/apache/helix/api/RunningInstance.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Spectator.java e69de29
>
> Diff: https://reviews.apache.org/r/13754/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhen Zhang
>
>
Re: Review Request 13754: [HELIX-109] Review Helix model package
Posted by Kishore Gopalakrishna <ki...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13754/#review25495
-----------------------------------------------------------
can we have separate packages for model, role, listeners ?
Lets avoid having strings, even things like cluster name, resource name, partition name should have concrete classes. Also having builder for every object is helpful. what do you think
- Kishore Gopalakrishna
On Aug. 24, 2013, 1:33 a.m., Zhen Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/13754/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2013, 1:33 a.m.)
>
>
> Review request for helix, Kanak Biscuitwala, Kishore Gopalakrishna, and Shi Lu.
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> Initial change for refactor helix model. Roughly follow the design at:
> https://cwiki.apache.org/confluence/display/HELIX/API+Redesign
>
> An example is shown in ClusterReader where using a Cluster instance, we can retrieve any information needed for the rebalancer. The change is incomplete.
>
>
> Diffs
> -----
>
> helix-core/src/main/java/org/apache/helix/api/Cluster.java e69de29
> helix-core/src/main/java/org/apache/helix/api/ClusterReader.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Controller.java e69de29
> helix-core/src/main/java/org/apache/helix/api/CurState.java e69de29
> helix-core/src/main/java/org/apache/helix/api/ExtView.java e69de29
> helix-core/src/main/java/org/apache/helix/api/HelixVersion.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Msg.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Participant.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Partition.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Resource.java e69de29
> helix-core/src/main/java/org/apache/helix/api/RunningInstance.java e69de29
> helix-core/src/main/java/org/apache/helix/api/Spectator.java e69de29
>
> Diff: https://reviews.apache.org/r/13754/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhen Zhang
>
>
Re: Review Request 13754: [HELIX-109] Review Helix model package
Posted by Zhen Zhang <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13754/
-----------------------------------------------------------
(Updated Aug. 24, 2013, 1:33 a.m.)
Review request for helix, Kanak Biscuitwala, Kishore Gopalakrishna, and Shi Lu.
Changes
-------
per discussion with Kanak, make Cluster instance read-only by using google guava immutable collection objects. mostly follow the following logical hierarchy:
- CLUSTER
- PARTICIPANT(*)
- CONFIG
- MESSAGE
- SPECTATOR(*)
- CONTROLLER(*)
- RESOURCE
- CONFIG
- IDEALSTATE
- EXTERNALVIEW
- CURRENTSTATE
Repository: helix-git
Description
-------
Initial change for refactor helix model. Roughly follow the design at:
https://cwiki.apache.org/confluence/display/HELIX/API+Redesign
An example is shown in ClusterReader where using a Cluster instance, we can retrieve any information needed for the rebalancer. The change is incomplete.
Diffs (updated)
-----
helix-core/src/main/java/org/apache/helix/api/Cluster.java e69de29
helix-core/src/main/java/org/apache/helix/api/ClusterReader.java e69de29
helix-core/src/main/java/org/apache/helix/api/Controller.java e69de29
helix-core/src/main/java/org/apache/helix/api/CurState.java e69de29
helix-core/src/main/java/org/apache/helix/api/ExtView.java e69de29
helix-core/src/main/java/org/apache/helix/api/HelixVersion.java e69de29
helix-core/src/main/java/org/apache/helix/api/Msg.java e69de29
helix-core/src/main/java/org/apache/helix/api/Participant.java e69de29
helix-core/src/main/java/org/apache/helix/api/Partition.java e69de29
helix-core/src/main/java/org/apache/helix/api/Resource.java e69de29
helix-core/src/main/java/org/apache/helix/api/RunningInstance.java e69de29
helix-core/src/main/java/org/apache/helix/api/Spectator.java e69de29
Diff: https://reviews.apache.org/r/13754/diff/
Testing
-------
Thanks,
Zhen Zhang