> On Dec. 6, 2013, 11:36 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/ZNRecord.java, line 593 > > <https://reviews.apache.org/r/16071/diff/2/?file=395120#file395120line593> > > > > are we changing the definition of how subtract works ?
yes. instead of subtracting an entire map, we are subtracting entries in the maps of map-field in znrecord. > On Dec. 6, 2013, 11:36 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/controller/stages/MessageSelectionStage.java, > > line 114 > > <https://reviews.apache.org/r/16071/diff/2/?file=395124#file395124line114> > > > > why do we allow adding resource without having statemodel? If this is > > the case where config exists but idealstate does not then we should not be > > processing that resource in the pipeline agree. task framework has this use case where resource config is used but the resource is not added as an ideal-state. need to exclude such resources from the pipeline. open a ticket for this: https://issues.apache.org/jira/browse/HELIX-338 > On Dec. 6, 2013, 11:36 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/api/accessor/ResourceAccessor.java, > > line 383 > > <https://reviews.apache.org/r/16071/diff/2/?file=395121#file395121line383> > > > > why is this static and made public? this is a utility function that converts between rebalancer-config and ideal-state (znrecord). we need access it from task-framework's user-defined rebalancer (TaskRebalancer) > On Dec. 6, 2013, 11:36 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/controller/stages/ResourceCurrentState.java, > > line 70 > > <https://reviews.apache.org/r/16071/diff/2/?file=395125#file395125line70> > > > > what is the String, should that be an Info class ? agree. create a ticket for this: https://issues.apache.org/jira/browse/HELIX-339 > On Dec. 6, 2013, 11:36 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/controller/stages/ResourceCurrentState.java, > > line 312 > > <https://reviews.apache.org/r/16071/diff/2/?file=395125#file395125line312> > > > > Can we have an Info class ? https://issues.apache.org/jira/browse/HELIX-339 > On Dec. 6, 2013, 11:36 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/model/CurrentState.java, line 338 > > <https://reviews.apache.org/r/16071/diff/2/?file=395128#file395128line338> > > > > Having info class is better no ? https://issues.apache.org/jira/browse/HELIX-339 > On Dec. 6, 2013, 11:36 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/task/TaskConfig.java, line 38 > > <https://reviews.apache.org/r/16071/diff/2/?file=395132#file395132line38> > > > > Shouldn't taskconfig and taskcontext be extending rebalancerconfig and > > rebalancercontext ? yes. Currently TaskRebalancer is not totally written following the RebalancerConfig/RebalancerContext model. We will refactor it. https://issues.apache.org/jira/browse/HELIX-340 > On Dec. 6, 2013, 11:36 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/messaging/handling/HelixStateTransitionHandler.java, > > line 404 > > <https://reviews.apache.org/r/16071/diff/2/?file=395126#file395126line404> > > > > Shouldn't keyBuilder have changed to accept typed objects yes. https://issues.apache.org/jira/browse/HELIX-341 > On Dec. 6, 2013, 11:36 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/messaging/handling/HelixStateTransitionHandler.java, > > line 359 > > <https://reviews.apache.org/r/16071/diff/2/?file=395126#file395126line359> > > > > what does instanceof void mean The Void class is an uninstantiable placeholder class to hold a reference to the Class object representing the Java keyword void. In Task framework state-model, some methods return a String instead of void. this is used to differentiate the transition method that is returning void from a string. > On Dec. 6, 2013, 11:36 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/messaging/handling/HelixStateTransitionHandler.java, > > line 187 > > <https://reviews.apache.org/r/16071/diff/2/?file=395126#file395126line187> > > > > should this be done always or we need a check that taskresult.getInfo > > is not null ? taskresult.getInfo() will be an empty string if not set. we can set info field only if it's not empty string. https://issues.apache.org/jira/browse/HELIX-342 > On Dec. 6, 2013, 11:36 p.m., Kishore Gopalakrishna wrote: > > helix-core/src/main/java/org/apache/helix/messaging/handling/HelixStateTransitionHandler.java, > > line 124 > > <https://reviews.apache.org/r/16071/diff/2/?file=395126#file395126line124> > > > > why are we resetting the info? How do we know that the controller has > > already processed that requested state? > > > > think of this scenario > > controllers sends msg 1 > > but before processing msg1, participant sets requested state, > > participant now reads msg1 and resets the requested state. Which means > > controller never saw the requested state. Is my understanding correct? > > > > requested-state is set when participant is processing the message, so the sequence would be, 1) controller sends message, 2) participant reset requested-state, 3) participant processes message, 4) participant set requested-state, 5) update current state, 6) delete message - Zhen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16071/#review29922 ----------------------------------------------------------- On Dec. 6, 2013, 7:34 p.m., Zhen Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16071/ > ----------------------------------------------------------- > > (Updated Dec. 6, 2013, 7:34 p.m.) > > > Review request for helix, Kanak Biscuitwala and Kishore Gopalakrishna. > > > Repository: helix-git > > > Description > ------- > > [HELIX-336] Add support for task framework > > > Diffs > ----- > > helix-core/src/main/java/org/apache/helix/ZNRecord.java 37cd5eb > > helix-core/src/main/java/org/apache/helix/api/accessor/ResourceAccessor.java > b308b98 > > helix-core/src/main/java/org/apache/helix/controller/stages/CurrentStateComputationStage.java > c036b14 > > helix-core/src/main/java/org/apache/helix/controller/stages/ExternalViewComputeStage.java > 7704378 > > helix-core/src/main/java/org/apache/helix/controller/stages/MessageSelectionStage.java > 9adc833 > > helix-core/src/main/java/org/apache/helix/controller/stages/ResourceCurrentState.java > f04afd0 > > helix-core/src/main/java/org/apache/helix/messaging/handling/HelixStateTransitionHandler.java > 8381f4a > > helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskResult.java > 22c4fcd > helix-core/src/main/java/org/apache/helix/model/CurrentState.java 5c9bcbc > > helix-core/src/main/java/org/apache/helix/participant/statemachine/StateModel.java > b88262b > helix-core/src/main/java/org/apache/helix/task/TargetState.java e69de29 > helix-core/src/main/java/org/apache/helix/task/Task.java e69de29 > helix-core/src/main/java/org/apache/helix/task/TaskConfig.java e69de29 > helix-core/src/main/java/org/apache/helix/task/TaskConstants.java e69de29 > helix-core/src/main/java/org/apache/helix/task/TaskContext.java e69de29 > helix-core/src/main/java/org/apache/helix/task/TaskDag.java e69de29 > helix-core/src/main/java/org/apache/helix/task/TaskDriver.java e69de29 > helix-core/src/main/java/org/apache/helix/task/TaskFactory.java e69de29 > helix-core/src/main/java/org/apache/helix/task/TaskPartitionState.java > e69de29 > helix-core/src/main/java/org/apache/helix/task/TaskRebalancer.java e69de29 > helix-core/src/main/java/org/apache/helix/task/TaskResult.java e69de29 > helix-core/src/main/java/org/apache/helix/task/TaskRunner.java e69de29 > helix-core/src/main/java/org/apache/helix/task/TaskState.java e69de29 > helix-core/src/main/java/org/apache/helix/task/TaskStateModel.java e69de29 > helix-core/src/main/java/org/apache/helix/task/TaskStateModelFactory.java > e69de29 > helix-core/src/main/java/org/apache/helix/task/TaskUtil.java e69de29 > helix-core/src/main/java/org/apache/helix/task/Workflow.java e69de29 > helix-core/src/main/java/org/apache/helix/task/WorkflowConfig.java e69de29 > helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java e69de29 > helix-core/src/main/java/org/apache/helix/task/beans/TaskBean.java e69de29 > helix-core/src/main/java/org/apache/helix/task/beans/WorkflowBean.java > e69de29 > helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java 1d02275 > > helix-core/src/main/java/org/apache/helix/tools/StateModelConfigGenerator.java > 8127626 > helix-core/src/test/java/org/apache/helix/TestZNRecord.java 9ff4849 > > helix-core/src/test/java/org/apache/helix/integration/ZkIntegrationTestBase.java > 9188e61 > > helix-core/src/test/java/org/apache/helix/integration/task/TestTaskRebalancer.java > e69de29 > > helix-core/src/test/java/org/apache/helix/integration/task/TestTaskRebalancerStopResume.java > e69de29 > helix-core/src/test/java/org/apache/helix/integration/task/TestUtil.java > e69de29 > > helix-core/src/test/java/org/apache/helix/integration/task/WorkflowGenerator.java > e69de29 > > Diff: https://reviews.apache.org/r/16071/diff/ > > > Testing > ------- > > all tests pass locally > > > Thanks, > > Zhen Zhang > >
