----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13878/#review25714 -----------------------------------------------------------
helix-core/src/main/java/org/apache/helix/api/ParticipantAccessor.java <https://reviews.apache.org/r/13878/#comment50219> shud be true helix-core/src/main/java/org/apache/helix/api/ParticipantAccessor.java <https://reviews.apache.org/r/13878/#comment50220> instead of insert push/send is probably appropriate/applies to all method related to message. helix-core/src/main/java/org/apache/helix/api/ParticipantAccessor.java <https://reviews.apache.org/r/13878/#comment50221> set is confusing, is this to update the message to read? if so then having a explicit method markMessagesAsRead or updateMessageStatus is probably more appropriate name helix-core/src/main/java/org/apache/helix/api/ParticipantAccessor.java <https://reviews.apache.org/r/13878/#comment50222> this is confusing, it feels like a method that starts the participant. register/connect or something like that can be used helix-core/src/main/java/org/apache/helix/api/ParticipantAccessor.java <https://reviews.apache.org/r/13878/#comment50223> Participant is used in two cases. one as a role and other to retrieve instanceinfo. you might want to have different name for the participant in logical model. helix-core/src/main/java/org/apache/helix/api/ParticipantAccessor.java <https://reviews.apache.org/r/13878/#comment50224> you dont need Participant in the method name. Order of arguments shud follow some heirarchy. resourceid, participantis, sessionId will be better. helix-core/src/main/java/org/apache/helix/api/PartitionId.java <https://reviews.apache.org/r/13878/#comment50225> partitionId should have two parts resourceId and partitionId. we should not assume that the form is resource_XX helix-core/src/main/java/org/apache/helix/api/RebalancerConfig.java <https://reviews.apache.org/r/13878/#comment50226> why RSCAssignment can we not call it ResourceAssignment ? helix-core/src/main/java/org/apache/helix/api/RebalancerConfig.java <https://reviews.apache.org/r/13878/#comment50227> how will rebalancer config know the resourceAssignment ? helix-core/src/main/java/org/apache/helix/api/RebalancerRef.java <https://reviews.apache.org/r/13878/#comment50228> please use logger helix-core/src/main/java/org/apache/helix/api/Resource.java <https://reviews.apache.org/r/13878/#comment50229> should use ExternalView from physical model helix-core/src/main/java/org/apache/helix/api/ResourceAccessor.java <https://reviews.apache.org/r/13878/#comment50230> typo in method name helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java <https://reviews.apache.org/r/13878/#comment50231> use stringify method on id class - Kishore Gopalakrishna On Aug. 29, 2013, 6:15 a.m., Zhen Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13878/ > ----------------------------------------------------------- > > (Updated Aug. 29, 2013, 6:15 a.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-admin-webapp/src/main/java/org/apache/helix/webapp/resources/SchedulerTasksResource.java > 59d9174 > > helix-admin-webapp/src/test/java/org/apache/helix/tools/TestResetPartitionState.java > c8099a4 > helix-agent/src/main/java/org/apache/helix/agent/AgentStateModel.java > 313f430 > helix-core/src/main/java/org/apache/helix/PropertyKey.java 0874958 > helix-core/src/main/java/org/apache/helix/api/Cluster.java 5b149d9 > helix-core/src/main/java/org/apache/helix/api/ClusterAccessor.java 1b43e6b > helix-core/src/main/java/org/apache/helix/api/ClusterReader.java 12a41ac > helix-core/src/main/java/org/apache/helix/api/Controller.java df28571 > helix-core/src/main/java/org/apache/helix/api/ControllerAccessor.java > fb3f844 > helix-core/src/main/java/org/apache/helix/api/CurState.java e66fb7a > helix-core/src/main/java/org/apache/helix/api/CurStateAccessor.java 1b8e2ce > helix-core/src/main/java/org/apache/helix/api/ExtViewAccessor.java 41406ff > helix-core/src/main/java/org/apache/helix/api/HelixVersion.java 84d0a8f > helix-core/src/main/java/org/apache/helix/api/Id.java 96ce15d > helix-core/src/main/java/org/apache/helix/api/MessageId.java e69de29 > helix-core/src/main/java/org/apache/helix/api/Msg.java ac7638d > helix-core/src/main/java/org/apache/helix/api/MsgId.java 88eb448 > helix-core/src/main/java/org/apache/helix/api/Participant.java e3eb68e > helix-core/src/main/java/org/apache/helix/api/ParticipantAccessor.java > 03e0992 > helix-core/src/main/java/org/apache/helix/api/PartitionId.java 3bec1ad > helix-core/src/main/java/org/apache/helix/api/RebalancerConfig.java 219e867 > helix-core/src/main/java/org/apache/helix/api/RebalancerRef.java 9011da9 > helix-core/src/main/java/org/apache/helix/api/Resource.java b76a0f8 > helix-core/src/main/java/org/apache/helix/api/ResourceAccessor.java b5a6516 > helix-core/src/main/java/org/apache/helix/api/RscAssignment.java 88e4ff6 > helix-core/src/main/java/org/apache/helix/api/RunningInstance.java 49c5ccf > helix-core/src/main/java/org/apache/helix/api/State.java b2000f2 > > helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java > 8e4e1ea > > helix-core/src/main/java/org/apache/helix/controller/rebalancer/AutoRebalancer.java > 9564e35 > > helix-core/src/main/java/org/apache/helix/controller/rebalancer/CustomRebalancer.java > 8557fa0 > > helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/ConstraintBasedAssignment.java > d2dbdef > > helix-core/src/main/java/org/apache/helix/controller/stages/ClusterDataCache.java > b90880e > > helix-core/src/main/java/org/apache/helix/controller/stages/CompatibilityCheckStage.java > d8f98ed > > helix-core/src/main/java/org/apache/helix/controller/stages/CurrentStateComputationStage.java > 6097432 > > helix-core/src/main/java/org/apache/helix/controller/stages/ExternalViewComputeStage.java > 35ef177 > > helix-core/src/main/java/org/apache/helix/controller/stages/MessageGenerationPhase.java > 92964e9 > > helix-core/src/main/java/org/apache/helix/controller/stages/MessageSelectionStage.java > 9a420aa > > helix-core/src/main/java/org/apache/helix/controller/stages/RebalanceIdealStateStage.java > d82ee2f > > helix-core/src/main/java/org/apache/helix/controller/stages/ResourceComputationStage.java > 51f0ec1 > > helix-core/src/main/java/org/apache/helix/controller/stages/TaskAssignmentStage.java > 192a645 > helix-core/src/main/java/org/apache/helix/manager/zk/ControllerManager.java > 1ed6dea > > helix-core/src/main/java/org/apache/helix/manager/zk/CurStateCarryOverUpdater.java > b96de18 > > helix-core/src/main/java/org/apache/helix/manager/zk/DefaultControllerMessageHandlerFactory.java > 5f6d083 > > helix-core/src/main/java/org/apache/helix/manager/zk/DefaultParticipantErrorMessageHandlerFactory.java > d2e56eb > > helix-core/src/main/java/org/apache/helix/manager/zk/DefaultSchedulerMessageHandlerFactory.java > 5451a81 > > helix-core/src/main/java/org/apache/helix/manager/zk/DistributedControllerManager.java > c9ad0f3 > > helix-core/src/main/java/org/apache/helix/manager/zk/DistributedLeaderElection.java > 0ab8342 > > helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManagerHelper.java > 70dd592 > helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java > 754df7b > > helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixDataAccessor.java > 025402d > helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java > 621c18b > helix-core/src/main/java/org/apache/helix/messaging/AsyncCallback.java > f9743a4 > > helix-core/src/main/java/org/apache/helix/messaging/DefaultMessagingService.java > 2eec354 > > helix-core/src/main/java/org/apache/helix/messaging/handling/AsyncCallbackService.java > c218a15 > > helix-core/src/main/java/org/apache/helix/messaging/handling/HelixStateTransitionHandler.java > 627babc > helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTask.java > d9f7ae2 > > helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java > 600a3ab > > helix-core/src/main/java/org/apache/helix/messaging/handling/MessageTimeoutTask.java > e1b4f0f > helix-core/src/main/java/org/apache/helix/model/ClusterConstraints.java > f69a7ce > helix-core/src/main/java/org/apache/helix/model/CurrentState.java 32854ab > helix-core/src/main/java/org/apache/helix/model/ExternalView.java d5f1afc > helix-core/src/main/java/org/apache/helix/model/IdealState.java e14940a > helix-core/src/main/java/org/apache/helix/model/InstanceConfig.java eb1c652 > helix-core/src/main/java/org/apache/helix/model/LiveInstance.java 75e0cf3 > helix-core/src/main/java/org/apache/helix/model/Message.java d599b8b > helix-core/src/main/java/org/apache/helix/model/ResourceAssignment.java > 2b3d14d > helix-core/src/main/java/org/apache/helix/model/StateModelDefinition.java > 7f08b6f > helix-core/src/main/java/org/apache/helix/model/Transition.java 2151c44 > > helix-core/src/main/java/org/apache/helix/model/builder/CurrentStateBuilder.java > e69de29 > > helix-core/src/main/java/org/apache/helix/model/builder/IdealStateBuilder.java > a7c0335 > > helix-core/src/main/java/org/apache/helix/monitoring/mbeans/ResourceMonitor.java > e24b41f > > helix-core/src/main/java/org/apache/helix/participant/DistClusterControllerElection.java > 25aada2 > > helix-core/src/main/java/org/apache/helix/participant/HelixStateMachineEngine.java > 31fcecf > > helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java > 9bba660 > helix-core/src/main/java/org/apache/helix/tools/ZkLogAnalyzer.java 11e1b66 > helix-core/src/main/java/org/apache/helix/util/RebalanceUtil.java 273adc3 > helix-core/src/main/java/org/apache/helix/util/StatusUpdateUtil.java > 02c39d1 > helix-core/src/test/java/org/apache/helix/ZkUnitTestBase.java abf75be > > helix-core/src/test/java/org/apache/helix/controller/stages/TestMsgSelectionStage.java > 820abbe > > helix-core/src/test/java/org/apache/helix/controller/stages/TestRebalancePipeline.java > fccd0c7 > > helix-core/src/test/java/org/apache/helix/controller/stages/TestResourceComputationStage.java > 6febe93 > helix-core/src/test/java/org/apache/helix/healthcheck/TestAddDropAlert.java > d5a1b08 > helix-core/src/test/java/org/apache/helix/healthcheck/TestExpandAlert.java > 69d1062 > helix-core/src/test/java/org/apache/helix/healthcheck/TestSimpleAlert.java > 1db5ddd > > helix-core/src/test/java/org/apache/helix/healthcheck/TestSimpleWildcardAlert.java > c5b55da > > helix-core/src/test/java/org/apache/helix/healthcheck/TestStalenessAlert.java > 2304b41 > > helix-core/src/test/java/org/apache/helix/healthcheck/TestWildcardAlert.java > a0456a7 > > helix-core/src/test/java/org/apache/helix/integration/TestAutoRebalance.java > 1943364 > > helix-core/src/test/java/org/apache/helix/integration/TestAutoRebalancePartitionLimit.java > 32cafcf > > helix-core/src/test/java/org/apache/helix/integration/TestCleanupExternalView.java > 781aa89 > > helix-core/src/test/java/org/apache/helix/integration/TestCustomizedIdealStateRebalancer.java > 55fc876 > helix-core/src/test/java/org/apache/helix/integration/TestDrop.java b1fcc60 > > helix-core/src/test/java/org/apache/helix/integration/TestEnablePartitionDuringDisable.java > 48cabbd > > helix-core/src/test/java/org/apache/helix/integration/TestHelixInstanceTag.java > 4484386 > > helix-core/src/test/java/org/apache/helix/integration/TestMessagePartitionStateMismatch.java > 487e689 > > helix-core/src/test/java/org/apache/helix/integration/TestMessagingService.java > 2354ebd > > helix-core/src/test/java/org/apache/helix/integration/TestResetPartitionState.java > 09e57c6 > > helix-core/src/test/java/org/apache/helix/integration/TestSchedulerMessage.java > 2c174c4 > > helix-core/src/test/java/org/apache/helix/integration/TestStateTransitionTimeout.java > edc10c6 > helix-core/src/test/java/org/apache/helix/integration/TestStatusUpdate.java > 4b92670 > > helix-core/src/test/java/org/apache/helix/manager/zk/TestZkClusterManager.java > 7809711 > > helix-core/src/test/java/org/apache/helix/messaging/TestAsyncCallbackSvc.java > 2be955f > > helix-core/src/test/java/org/apache/helix/messaging/handling/TestHelixTaskExecutor.java > 1ff6595 > > helix-core/src/test/java/org/apache/helix/mock/participant/ErrTransition.java > 301cd62 > helix-core/src/test/java/org/apache/helix/tools/TestHelixAdminCli.java > c58f94d > > helix-examples/src/main/java/org/apache/helix/examples/MasterSlaveStateModelFactory.java > affbea8 > helix-examples/src/main/java/org/apache/helix/examples/Quickstart.java > b80d458 > > recipes/distributed-lock-manager/src/main/java/org/apache/helix/lockmanager/LockManagerDemo.java > b6c54db > > recipes/rsync-replicated-file-system/src/main/java/org/apache/helix/filestore/FileStoreStateModel.java > 6809a87 > > recipes/task-execution/src/main/java/org/apache/helix/taskexecution/Task.java > 0cc8bba > > Diff: https://reviews.apache.org/r/13878/diff/ > > > Testing > ------- > > > Thanks, > > Zhen Zhang > >
