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/12/06 02:21:23 UTC
Review Request 16071: [HELIX-336] Add support for task framework
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16071/
-----------------------------------------------------------
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
Re: Review Request 16071: [HELIX-336] Add support for task framework
Posted by Zhen Zhang <ne...@gmail.com>.
> On Dec. 6, 2013, 1:33 a.m., Kanak Biscuitwala wrote:
> > helix-core/src/main/java/org/apache/helix/controller/stages/MessageSelectionStage.java, line 114
> > <https://reviews.apache.org/r/16071/diff/1/?file=394827#file394827line114>
> >
> > I don't really understand this error message. Can you clarify?
in task framework, sometimes we add a resource config but we don't add the resource in ideal-states. fix the log info.
> On Dec. 6, 2013, 1:33 a.m., Kanak Biscuitwala wrote:
> > helix-core/src/main/java/org/apache/helix/task/TaskRunner.java, line 29
> > <https://reviews.apache.org/r/16071/diff/1/?file=394844#file394844line29>
> >
> > Remove all author tags since Apache doesn't allow them
fixed it. remove both author and version tags
> On Dec. 6, 2013, 1:33 a.m., Kanak Biscuitwala wrote:
> > helix-core/src/main/java/org/apache/helix/task/TaskUtil.java, line 47
> > <https://reviews.apache.org/r/16071/diff/1/?file=394848#file394848line47>
> >
> > In the code, we've been trying to get away from using string constants and using enums instead. Might want to change this to stay consistent.
fixed it
> On Dec. 6, 2013, 1:33 a.m., Kanak Biscuitwala wrote:
> > helix-core/src/main/java/org/apache/helix/task/Workflow.java, line 42
> > <https://reviews.apache.org/r/16071/diff/1/?file=394849#file394849line42>
> >
> > Same as before, enum might be better
fixed it
- Zhen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16071/#review29860
-----------------------------------------------------------
On Dec. 6, 2013, 1:21 a.m., Zhen Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16071/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2013, 1:21 a.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
>
>
Re: Review Request 16071: [HELIX-336] Add support for task framework
Posted by Kanak Biscuitwala <ka...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16071/#review29860
-----------------------------------------------------------
helix-core/src/main/java/org/apache/helix/controller/stages/MessageSelectionStage.java
<https://reviews.apache.org/r/16071/#comment57321>
I don't really understand this error message. Can you clarify?
helix-core/src/main/java/org/apache/helix/task/TaskRunner.java
<https://reviews.apache.org/r/16071/#comment57322>
Remove all author tags since Apache doesn't allow them
helix-core/src/main/java/org/apache/helix/task/TaskStateModel.java
<https://reviews.apache.org/r/16071/#comment57323>
Would be nice to have some comment here explaining how exactly this differs from a standard state model
helix-core/src/main/java/org/apache/helix/task/TaskUtil.java
<https://reviews.apache.org/r/16071/#comment57324>
In the code, we've been trying to get away from using string constants and using enums instead. Might want to change this to stay consistent.
helix-core/src/main/java/org/apache/helix/task/Workflow.java
<https://reviews.apache.org/r/16071/#comment57325>
Same as before, enum might be better
- Kanak Biscuitwala
On Dec. 5, 2013, 5:21 p.m., Zhen Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16071/
> -----------------------------------------------------------
>
> (Updated Dec. 5, 2013, 5:21 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
>
>
Re: Review Request 16071: [HELIX-336] Add support for task framework
Posted by Zhen Zhang <ne...@gmail.com>.
> 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
>
>
Re: Review Request 16071: [HELIX-336] Add support for task framework
Posted by Kishore Gopalakrishna <ki...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16071/#review29922
-----------------------------------------------------------
Ship it!
Please fix what ever you can and file jira for big changes/future enhancements
helix-core/src/main/java/org/apache/helix/ZNRecord.java
<https://reviews.apache.org/r/16071/#comment57392>
are we changing the definition of how subtract works ?
helix-core/src/main/java/org/apache/helix/api/accessor/ResourceAccessor.java
<https://reviews.apache.org/r/16071/#comment57393>
why is this static and made public?
helix-core/src/main/java/org/apache/helix/controller/stages/MessageSelectionStage.java
<https://reviews.apache.org/r/16071/#comment57394>
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
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceCurrentState.java
<https://reviews.apache.org/r/16071/#comment57395>
what is the String, should that be an Info class ?
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceCurrentState.java
<https://reviews.apache.org/r/16071/#comment57396>
Can we have an Info class ?
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixStateTransitionHandler.java
<https://reviews.apache.org/r/16071/#comment57397>
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?
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixStateTransitionHandler.java
<https://reviews.apache.org/r/16071/#comment57398>
should this be done always or we need a check that taskresult.getInfo is not null ?
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixStateTransitionHandler.java
<https://reviews.apache.org/r/16071/#comment57399>
what does instanceof void mean
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixStateTransitionHandler.java
<https://reviews.apache.org/r/16071/#comment57400>
Shouldn't keyBuilder have changed to accept typed objects
helix-core/src/main/java/org/apache/helix/model/CurrentState.java
<https://reviews.apache.org/r/16071/#comment57406>
Having info class is better no ?
helix-core/src/main/java/org/apache/helix/task/TaskConfig.java
<https://reviews.apache.org/r/16071/#comment57407>
Shouldn't taskconfig and taskcontext be extending rebalancerconfig and rebalancercontext ?
helix-core/src/main/java/org/apache/helix/task/TaskDag.java
<https://reviews.apache.org/r/16071/#comment57408>
Nice
- Kishore Gopalakrishna
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
>
>
Re: Review Request 16071: [HELIX-336] Add support for task framework
Posted by Kanak Biscuitwala <ka...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16071/#review29904
-----------------------------------------------------------
Ship it!
I still see a lot of string constants, which I would recommend replacing with enums. Other than that, looks good.
- Kanak Biscuitwala
On Dec. 6, 2013, 11:34 a.m., Zhen Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16071/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2013, 11:34 a.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
>
>
Re: Review Request 16071: [HELIX-336] Add support for task framework
Posted by Zhen Zhang <ne...@gmail.com>.
-----------------------------------------------------------
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.
Changes
-------
update the diff
Repository: helix-git
Description
-------
[HELIX-336] Add support for task framework
Diffs (updated)
-----
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