----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13878/#review25674 -----------------------------------------------------------
Ship it! This change isn't done; please submit it to a new branch. The plan is to work on a feature branch until the code is complete, and then we'll do an additional review before merging into master. - Kanak Biscuitwala On Aug. 28, 2013, 6:16 p.m., Zhen Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13878/ > ----------------------------------------------------------- > > (Updated Aug. 28, 2013, 6:16 p.m.) > > > Review request for helix, Kanak Biscuitwala and Kishore Gopalakrishna. > > > Bugs: HELIX-109 > > > Repository: helix-git > > > Description > ------- > > commit 394e06a2894f92b0a9fb2787a57d3a0ef7468825 > Author: zzhang <[email protected]> > Date: Tue Aug 27 18:03:33 2013 -0700 > > [HELIX-109] Review Helix model package, initial changes > > :000000 100644 0000000... f92b14f... A > helix-core/src/main/java/org/apache/helix/api/Cluster.java > :000000 100644 0000000... 3968376... A > helix-core/src/main/java/org/apache/helix/api/ClusterId.java > :000000 100644 0000000... 643e466... A > helix-core/src/main/java/org/apache/helix/api/ClusterReader.java > :000000 100644 0000000... bab45f8... A > helix-core/src/main/java/org/apache/helix/api/ClusterSnapshot.java > :000000 100644 0000000... 5266a81... A > helix-core/src/main/java/org/apache/helix/api/Controller.java > :000000 100644 0000000... 1ff3bd3... A > helix-core/src/main/java/org/apache/helix/api/ControllerId.java > :000000 100644 0000000... 479c33b... A > helix-core/src/main/java/org/apache/helix/api/ControllerSnapshot.java > :000000 100644 0000000... e66fb7a... A > helix-core/src/main/java/org/apache/helix/api/CurState.java > :000000 100644 0000000... 3a5d4ac... A > helix-core/src/main/java/org/apache/helix/api/ExtView.java > :000000 100644 0000000... 84d0a8f... A > helix-core/src/main/java/org/apache/helix/api/HelixVersion.java > :000000 100644 0000000... d65c3d7... A > helix-core/src/main/java/org/apache/helix/api/Id.java > :000000 100644 0000000... ac7638d... A > helix-core/src/main/java/org/apache/helix/api/Msg.java > :000000 100644 0000000... c61a5bf... A > helix-core/src/main/java/org/apache/helix/api/MsgId.java > :000000 100644 0000000... 16ed316... A > helix-core/src/main/java/org/apache/helix/api/Participant.java > :000000 100644 0000000... eadc615... A > helix-core/src/main/java/org/apache/helix/api/ParticipantId.java > :000000 100644 0000000... 48bdfff... A > helix-core/src/main/java/org/apache/helix/api/ParticipantSnapshot.java > :000000 100644 0000000... c903493... A > helix-core/src/main/java/org/apache/helix/api/Partition.java > :000000 100644 0000000... ace767d... A > helix-core/src/main/java/org/apache/helix/api/PartitionId.java > :000000 100644 0000000... e244032... A > helix-core/src/main/java/org/apache/helix/api/ProcId.java > :000000 100644 0000000... 3d1c69f... A > helix-core/src/main/java/org/apache/helix/api/RebalancerRef.java > :000000 100644 0000000... dc615de... A > helix-core/src/main/java/org/apache/helix/api/Resource.java > :000000 100644 0000000... 32cdf4f... A > helix-core/src/main/java/org/apache/helix/api/ResourceId.java > :000000 100644 0000000... a5e04ff... A > helix-core/src/main/java/org/apache/helix/api/ResourceSnapshot.java > :000000 100644 0000000... a7b6316... A > helix-core/src/main/java/org/apache/helix/api/RscAssignment.java > :000000 100644 0000000... 49c5ccf... A > helix-core/src/main/java/org/apache/helix/api/RunningInstance.java > :000000 100644 0000000... 9a070c0... A > helix-core/src/main/java/org/apache/helix/api/SessionId.java > :000000 100644 0000000... a4ab2c5... A > helix-core/src/main/java/org/apache/helix/api/Spectator.java > :000000 100644 0000000... bd0150c... A > helix-core/src/main/java/org/apache/helix/api/SpectatorId.java > :000000 100644 0000000... 947c767... A > helix-core/src/main/java/org/apache/helix/api/SpectatorSnapshot.java > :000000 100644 0000000... ebd31cc... A > helix-core/src/main/java/org/apache/helix/api/State.java > :000000 100644 0000000... fe2b3e0... A > helix-core/src/main/java/org/apache/helix/api/StateModelDefId.java > > > close the other jira. paste the original comments here: > 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 3 days, 19 hours ago (Aug. 24, 2013, 5:50 a.m.) > - 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. > Zhen Zhang 2 days, 20 hours ago (Aug. 25, 2013, 4:56 a.m.) > 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? > > > Diffs > ----- > > helix-core/src/main/java/org/apache/helix/api/Cluster.java PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/ClusterAccessor.java > PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/ClusterConfig.java > PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/ClusterId.java PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/ClusterReader.java > PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/Controller.java PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/ControllerAccessor.java > PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/ControllerId.java > PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/CurState.java PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/CurStateAccessor.java > PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/ExtView.java PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/ExtViewAccessor.java > PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/HelixVersion.java > PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/Id.java PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/Msg.java PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/MsgId.java PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/Participant.java PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/ParticipantAccessor.java > PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/ParticipantId.java > PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/Partition.java PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/PartitionId.java PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/ProcId.java PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/RebalancerConfig.java > PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/RebalancerRef.java > PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/Resource.java PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/ResourceAccessor.java > PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/ResourceId.java PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/RscAssignment.java > PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/RunningInstance.java > PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/SessionId.java PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/Spectator.java PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/SpectatorId.java PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/State.java PRE-CREATION > helix-core/src/main/java/org/apache/helix/api/StateModelDefId.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/13878/diff/ > > > Testing > ------- > > > Thanks, > > Zhen Zhang > >
