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 2014/04/16 03:35:36 UTC
Review Request 20401: [HELIX-94] Add the ability to enable and disable a
resource
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20401/
-----------------------------------------------------------
Review request for helix, Kanak Biscuitwala and Kishore Gopalakrishna.
Bugs: HELIX-94
Repository: helix-git
Description
-------
[HELIX-94] Add the ability to enable and disable a resource
Diffs
-----
helix-admin-webapp/src/main/java/org/apache/helix/webapp/resources/JsonParameters.java c8e8fc6
helix-admin-webapp/src/main/java/org/apache/helix/webapp/resources/ResourceGroupResource.java 365b212
helix-admin-webapp/src/test/java/org/apache/helix/webapp/TestDisableResource.java e69de29
helix-core/src/main/java/org/apache/helix/HelixAdmin.java d5c62fa
helix-core/src/main/java/org/apache/helix/controller/rebalancer/AutoRebalancer.java 745a9c9
helix-core/src/main/java/org/apache/helix/controller/rebalancer/CustomRebalancer.java 69037d9
helix-core/src/main/java/org/apache/helix/controller/rebalancer/SemiAutoRebalancer.java 420e7ab
helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/ConstraintBasedAssignment.java 3fd52f4
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java 169b2bb
helix-core/src/main/java/org/apache/helix/model/IdealState.java 7a4fcad
helix-core/src/main/java/org/apache/helix/participant/HelixCustomCodeRunner.java 6a2490a
helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java a39e571
helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java c93c51e
helix-core/src/test/java/org/apache/helix/integration/TestDisableCustomCodeRunner.java e69de29
helix-core/src/test/java/org/apache/helix/integration/TestDisableResource.java e69de29
helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java 4b3764f
helix-core/src/test/java/org/apache/helix/tools/TestClusterSetup.java 7190968
Diff: https://reviews.apache.org/r/20401/diff/
Testing
-------
Thanks,
Zhen Zhang
Re: Review Request 20401: [HELIX-94] Add the ability to enable and disable a
resource
Posted by Zhen Zhang <ne...@gmail.com>.
> On April 16, 2014, 5:01 a.m., Kanak Biscuitwala wrote:
> > helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java, line 205
> > <https://reviews.apache.org/r/20401/diff/1/?file=560738#file560738line205>
> >
> > Use BaseDataAccessor<ZNRecord> baseAccessor = ...
fixed
> On April 16, 2014, 5:01 a.m., Kanak Biscuitwala wrote:
> > helix-core/src/main/java/org/apache/helix/model/IdealState.java, line 54
> > <https://reviews.apache.org/r/20401/diff/1/?file=560739#file560739line54>
> >
> > I would call this HELIX_ENABLED to stay consistent with instance enable/disable
fixed
> On April 16, 2014, 5:01 a.m., Kanak Biscuitwala wrote:
> > helix-core/src/main/java/org/apache/helix/model/IdealState.java, line 522
> > <https://reviews.apache.org/r/20401/diff/1/?file=560739#file560739line522>
> >
> > I would fold this into a single setEnabled(boolean) method to be symmetric with the instance enable behavior
fixed
> On April 16, 2014, 5:01 a.m., Kanak Biscuitwala wrote:
> > helix-core/src/main/java/org/apache/helix/participant/HelixCustomCodeRunner.java, line 106
> > <https://reviews.apache.org/r/20401/diff/1/?file=560740#file560740line106>
> >
> > Add a javadoc explaining why this method is useful.
fixed
> On April 16, 2014, 5:01 a.m., Kanak Biscuitwala wrote:
> > helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java, line 301
> > <https://reviews.apache.org/r/20401/diff/1/?file=560745#file560745line301>
> >
> > HelixAdmin tool = ...
fixed
> On April 16, 2014, 5:01 a.m., Kanak Biscuitwala wrote:
> > helix-core/src/test/java/org/apache/helix/integration/TestDisableCustomCodeRunner.java, line 59
> > <https://reviews.apache.org/r/20401/diff/1/?file=560743#file560743line59>
> >
> > Differentiate between init/callback and finalize
fixed
> On April 16, 2014, 5:01 a.m., Kanak Biscuitwala wrote:
> > helix-core/src/test/java/org/apache/helix/integration/TestDisableResource.java, line 93
> > <https://reviews.apache.org/r/20401/diff/1/?file=560744#file560744line93>
> >
> > You might want to verify by polling here since external view can change multiple times.
fixed
- Zhen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20401/#review40489
-----------------------------------------------------------
On April 16, 2014, 1:35 a.m., Zhen Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20401/
> -----------------------------------------------------------
>
> (Updated April 16, 2014, 1:35 a.m.)
>
>
> Review request for helix, Kanak Biscuitwala and Kishore Gopalakrishna.
>
>
> Bugs: HELIX-94
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> [HELIX-94] Add the ability to enable and disable a resource
>
>
> Diffs
> -----
>
> helix-admin-webapp/src/main/java/org/apache/helix/webapp/resources/JsonParameters.java c8e8fc6
> helix-admin-webapp/src/main/java/org/apache/helix/webapp/resources/ResourceGroupResource.java 365b212
> helix-admin-webapp/src/test/java/org/apache/helix/webapp/TestDisableResource.java e69de29
> helix-core/src/main/java/org/apache/helix/HelixAdmin.java d5c62fa
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/AutoRebalancer.java 745a9c9
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/CustomRebalancer.java 69037d9
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/SemiAutoRebalancer.java 420e7ab
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/ConstraintBasedAssignment.java 3fd52f4
> helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java 169b2bb
> helix-core/src/main/java/org/apache/helix/model/IdealState.java 7a4fcad
> helix-core/src/main/java/org/apache/helix/participant/HelixCustomCodeRunner.java 6a2490a
> helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java a39e571
> helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java c93c51e
> helix-core/src/test/java/org/apache/helix/integration/TestDisableCustomCodeRunner.java e69de29
> helix-core/src/test/java/org/apache/helix/integration/TestDisableResource.java e69de29
> helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java 4b3764f
> helix-core/src/test/java/org/apache/helix/mock/participant/MockMSModelFactory.java ff7a455
> helix-core/src/test/java/org/apache/helix/tools/TestClusterSetup.java 7190968
>
> Diff: https://reviews.apache.org/r/20401/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhen Zhang
>
>
Re: Review Request 20401: [HELIX-94] Add the ability to enable and disable a
resource
Posted by Kanak Biscuitwala <ka...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20401/#review40489
-----------------------------------------------------------
Good job with the test coverage. Mostly minor comments.
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java
<https://reviews.apache.org/r/20401/#comment73516>
Use BaseDataAccessor<ZNRecord> baseAccessor = ...
helix-core/src/main/java/org/apache/helix/model/IdealState.java
<https://reviews.apache.org/r/20401/#comment73526>
I would call this HELIX_ENABLED to stay consistent with instance enable/disable
helix-core/src/main/java/org/apache/helix/model/IdealState.java
<https://reviews.apache.org/r/20401/#comment73518>
I would fold this into a single setEnabled(boolean) method to be symmetric with the instance enable behavior
helix-core/src/main/java/org/apache/helix/participant/HelixCustomCodeRunner.java
<https://reviews.apache.org/r/20401/#comment73527>
Add a javadoc explaining why this method is useful.
helix-core/src/test/java/org/apache/helix/integration/TestDisableCustomCodeRunner.java
<https://reviews.apache.org/r/20401/#comment73529>
Differentiate between init/callback and finalize
helix-core/src/test/java/org/apache/helix/integration/TestDisableResource.java
<https://reviews.apache.org/r/20401/#comment73532>
You might want to verify by polling here since external view can change multiple times.
helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java
<https://reviews.apache.org/r/20401/#comment73533>
HelixAdmin tool = ...
- Kanak Biscuitwala
On April 15, 2014, 6:35 p.m., Zhen Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20401/
> -----------------------------------------------------------
>
> (Updated April 15, 2014, 6:35 p.m.)
>
>
> Review request for helix, Kanak Biscuitwala and Kishore Gopalakrishna.
>
>
> Bugs: HELIX-94
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> [HELIX-94] Add the ability to enable and disable a resource
>
>
> Diffs
> -----
>
> helix-admin-webapp/src/main/java/org/apache/helix/webapp/resources/JsonParameters.java c8e8fc6
> helix-admin-webapp/src/main/java/org/apache/helix/webapp/resources/ResourceGroupResource.java 365b212
> helix-admin-webapp/src/test/java/org/apache/helix/webapp/TestDisableResource.java e69de29
> helix-core/src/main/java/org/apache/helix/HelixAdmin.java d5c62fa
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/AutoRebalancer.java 745a9c9
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/CustomRebalancer.java 69037d9
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/SemiAutoRebalancer.java 420e7ab
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/ConstraintBasedAssignment.java 3fd52f4
> helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java 169b2bb
> helix-core/src/main/java/org/apache/helix/model/IdealState.java 7a4fcad
> helix-core/src/main/java/org/apache/helix/participant/HelixCustomCodeRunner.java 6a2490a
> helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java a39e571
> helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java c93c51e
> helix-core/src/test/java/org/apache/helix/integration/TestDisableCustomCodeRunner.java e69de29
> helix-core/src/test/java/org/apache/helix/integration/TestDisableResource.java e69de29
> helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java 4b3764f
> helix-core/src/test/java/org/apache/helix/tools/TestClusterSetup.java 7190968
>
> Diff: https://reviews.apache.org/r/20401/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhen Zhang
>
>
Re: Review Request 20401: [HELIX-94] Add the ability to enable and disable a
resource
Posted by Kanak Biscuitwala <ka...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20401/#review40576
-----------------------------------------------------------
Ship it!
Ship It!
- Kanak Biscuitwala
On April 16, 2014, 10:42 a.m., Zhen Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20401/
> -----------------------------------------------------------
>
> (Updated April 16, 2014, 10:42 a.m.)
>
>
> Review request for helix, Kanak Biscuitwala and Kishore Gopalakrishna.
>
>
> Bugs: HELIX-94
>
>
> Repository: helix-git
>
>
> Description
> -------
>
> [HELIX-94] Add the ability to enable and disable a resource
>
>
> Diffs
> -----
>
> helix-admin-webapp/src/main/java/org/apache/helix/webapp/resources/JsonParameters.java c8e8fc6
> helix-admin-webapp/src/main/java/org/apache/helix/webapp/resources/ResourceGroupResource.java 365b212
> helix-admin-webapp/src/test/java/org/apache/helix/webapp/TestDisableResource.java e69de29
> helix-core/src/main/java/org/apache/helix/HelixAdmin.java d5c62fa
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/AutoRebalancer.java 745a9c9
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/CustomRebalancer.java 69037d9
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/SemiAutoRebalancer.java 420e7ab
> helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/ConstraintBasedAssignment.java 3fd52f4
> helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java 169b2bb
> helix-core/src/main/java/org/apache/helix/model/IdealState.java 7a4fcad
> helix-core/src/main/java/org/apache/helix/participant/HelixCustomCodeRunner.java 6a2490a
> helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java a39e571
> helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java c93c51e
> helix-core/src/test/java/org/apache/helix/integration/TestDisableCustomCodeRunner.java e69de29
> helix-core/src/test/java/org/apache/helix/integration/TestDisableResource.java e69de29
> helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java 4b3764f
> helix-core/src/test/java/org/apache/helix/mock/participant/MockMSModelFactory.java ff7a455
> helix-core/src/test/java/org/apache/helix/tools/TestClusterSetup.java 7190968
>
> Diff: https://reviews.apache.org/r/20401/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhen Zhang
>
>
Re: Review Request 20401: [HELIX-94] Add the ability to enable and disable a
resource
Posted by Zhen Zhang <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20401/
-----------------------------------------------------------
(Updated April 16, 2014, 5:42 p.m.)
Review request for helix, Kanak Biscuitwala and Kishore Gopalakrishna.
Changes
-------
fixed review comment issue
Bugs: HELIX-94
Repository: helix-git
Description
-------
[HELIX-94] Add the ability to enable and disable a resource
Diffs (updated)
-----
helix-admin-webapp/src/main/java/org/apache/helix/webapp/resources/JsonParameters.java c8e8fc6
helix-admin-webapp/src/main/java/org/apache/helix/webapp/resources/ResourceGroupResource.java 365b212
helix-admin-webapp/src/test/java/org/apache/helix/webapp/TestDisableResource.java e69de29
helix-core/src/main/java/org/apache/helix/HelixAdmin.java d5c62fa
helix-core/src/main/java/org/apache/helix/controller/rebalancer/AutoRebalancer.java 745a9c9
helix-core/src/main/java/org/apache/helix/controller/rebalancer/CustomRebalancer.java 69037d9
helix-core/src/main/java/org/apache/helix/controller/rebalancer/SemiAutoRebalancer.java 420e7ab
helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/ConstraintBasedAssignment.java 3fd52f4
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java 169b2bb
helix-core/src/main/java/org/apache/helix/model/IdealState.java 7a4fcad
helix-core/src/main/java/org/apache/helix/participant/HelixCustomCodeRunner.java 6a2490a
helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java a39e571
helix-core/src/test/java/org/apache/helix/controller/strategy/TestAutoRebalanceStrategy.java c93c51e
helix-core/src/test/java/org/apache/helix/integration/TestDisableCustomCodeRunner.java e69de29
helix-core/src/test/java/org/apache/helix/integration/TestDisableResource.java e69de29
helix-core/src/test/java/org/apache/helix/manager/zk/TestZkHelixAdmin.java 4b3764f
helix-core/src/test/java/org/apache/helix/mock/participant/MockMSModelFactory.java ff7a455
helix-core/src/test/java/org/apache/helix/tools/TestClusterSetup.java 7190968
Diff: https://reviews.apache.org/r/20401/diff/
Testing
-------
Thanks,
Zhen Zhang