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/10/01 03:00:24 UTC

Review Request 26212: [HELIX-519] Add integration tests to ensure that "kill-switch" for Helix tasks work as expected

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26212/
-----------------------------------------------------------

Review request for helix, Kanak Biscuitwala and Shi Lu.


Bugs: 519


Repository: helix-git


Description
-------

[HELIX-519] Add integration tests to ensure that "kill-switch" for Helix tasks work as expected


Diffs
-----

  helix-core/src/main/java/org/apache/helix/task/TaskDriver.java 31d785b 
  helix-core/src/test/java/org/apache/helix/integration/task/TestTaskRebalancerStopResume.java 4e0d92a 
  helix-core/src/test/java/org/apache/helix/integration/task/TestUtil.java f599920 

Diff: https://reviews.apache.org/r/26212/diff/


Testing
-------

mvn test


Thanks,

Zhen Zhang

Re: Review Request 26212: [HELIX-519] Add integration tests to ensure that "kill-switch" for Helix tasks work as expected

Posted by Zhen Zhang <ne...@gmail.com>.

> On Oct. 1, 2014, 4:19 a.m., Kanak Biscuitwala wrote:
> > helix-core/src/main/java/org/apache/helix/task/TaskDriver.java, line 383
> > <https://reviews.apache.org/r/26212/diff/1/?file=709970#file709970line383>
> >
> >     Why this change?

For TaskDriver#setTaskTargetState:

  private void setTaskTargetState(String jobResource, TargetState state) {
    setSingleTaskTargetState(jobResource, state);

    // For recurring schedules, child workflows must also be handled
    HelixDataAccessor accessor = _manager.getHelixDataAccessor();
    List<String> resources = accessor.getChildNames(accessor.keyBuilder().resourceConfigs());
    for (String resource : resources) {
      String prefix = resource + "_" + TaskConstants.SCHEDULED;
      if (resource.startsWith(prefix)) {
        setSingleTaskTargetState(resource, state);
      }
    }
  }
  
  If I understand correctly, prefix should be jobResource + "_" + TaskConstants.SCHEDULED instead of resource + "_" + TaskConstants.SCHEDULED, otherwise resource.startsWith(prefix) doesn't seem to make sense?


- Zhen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26212/#review55051
-----------------------------------------------------------


On Oct. 1, 2014, 1 a.m., Zhen Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26212/
> -----------------------------------------------------------
> 
> (Updated Oct. 1, 2014, 1 a.m.)
> 
> 
> Review request for helix, Kanak Biscuitwala and Shi Lu.
> 
> 
> Bugs: 519
> 
> 
> Repository: helix-git
> 
> 
> Description
> -------
> 
> [HELIX-519] Add integration tests to ensure that "kill-switch" for Helix tasks work as expected
> 
> 
> Diffs
> -----
> 
>   helix-core/src/main/java/org/apache/helix/task/TaskDriver.java 31d785b 
>   helix-core/src/test/java/org/apache/helix/integration/task/TestTaskRebalancerStopResume.java 4e0d92a 
>   helix-core/src/test/java/org/apache/helix/integration/task/TestUtil.java f599920 
> 
> Diff: https://reviews.apache.org/r/26212/diff/
> 
> 
> Testing
> -------
> 
> mvn test
> 
> 
> Thanks,
> 
> Zhen Zhang
> 
>

Re: Review Request 26212: [HELIX-519] Add integration tests to ensure that "kill-switch" for Helix tasks work as expected

Posted by Kanak Biscuitwala <ka...@apache.org>.

> On Sept. 30, 2014, 9:19 p.m., Kanak Biscuitwala wrote:
> > helix-core/src/main/java/org/apache/helix/task/TaskDriver.java, line 383
> > <https://reviews.apache.org/r/26212/diff/1/?file=709970#file709970line383>
> >
> >     Why this change?
> 
> Zhen Zhang wrote:
>     For TaskDriver#setTaskTargetState:
>     
>       private void setTaskTargetState(String jobResource, TargetState state) {
>         setSingleTaskTargetState(jobResource, state);
>     
>         // For recurring schedules, child workflows must also be handled
>         HelixDataAccessor accessor = _manager.getHelixDataAccessor();
>         List<String> resources = accessor.getChildNames(accessor.keyBuilder().resourceConfigs());
>         for (String resource : resources) {
>           String prefix = resource + "_" + TaskConstants.SCHEDULED;
>           if (resource.startsWith(prefix)) {
>             setSingleTaskTargetState(resource, state);
>           }
>         }
>       }
>       
>       If I understand correctly, prefix should be jobResource + "_" + TaskConstants.SCHEDULED instead of resource + "_" + TaskConstants.SCHEDULED, otherwise resource.startsWith(prefix) doesn't seem to make sense?

So this is my bad, but this method should be called setWorkflowTargetState, and the first argument should be workflowResource, and then yes your change is correct.


- Kanak


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26212/#review55051
-----------------------------------------------------------


On Sept. 30, 2014, 6 p.m., Zhen Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26212/
> -----------------------------------------------------------
> 
> (Updated Sept. 30, 2014, 6 p.m.)
> 
> 
> Review request for helix, Kanak Biscuitwala and Shi Lu.
> 
> 
> Bugs: 519
> 
> 
> Repository: helix-git
> 
> 
> Description
> -------
> 
> [HELIX-519] Add integration tests to ensure that "kill-switch" for Helix tasks work as expected
> 
> 
> Diffs
> -----
> 
>   helix-core/src/main/java/org/apache/helix/task/TaskDriver.java 31d785b 
>   helix-core/src/test/java/org/apache/helix/integration/task/TestTaskRebalancerStopResume.java 4e0d92a 
>   helix-core/src/test/java/org/apache/helix/integration/task/TestUtil.java f599920 
> 
> Diff: https://reviews.apache.org/r/26212/diff/
> 
> 
> Testing
> -------
> 
> mvn test
> 
> 
> Thanks,
> 
> Zhen Zhang
> 
>

Re: Review Request 26212: [HELIX-519] Add integration tests to ensure that "kill-switch" for Helix tasks work as expected

Posted by Kanak Biscuitwala <ka...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26212/#review55051
-----------------------------------------------------------



helix-core/src/main/java/org/apache/helix/task/TaskDriver.java
<https://reviews.apache.org/r/26212/#comment95390>

    Why this change?


- Kanak Biscuitwala


On Sept. 30, 2014, 6 p.m., Zhen Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26212/
> -----------------------------------------------------------
> 
> (Updated Sept. 30, 2014, 6 p.m.)
> 
> 
> Review request for helix, Kanak Biscuitwala and Shi Lu.
> 
> 
> Bugs: 519
> 
> 
> Repository: helix-git
> 
> 
> Description
> -------
> 
> [HELIX-519] Add integration tests to ensure that "kill-switch" for Helix tasks work as expected
> 
> 
> Diffs
> -----
> 
>   helix-core/src/main/java/org/apache/helix/task/TaskDriver.java 31d785b 
>   helix-core/src/test/java/org/apache/helix/integration/task/TestTaskRebalancerStopResume.java 4e0d92a 
>   helix-core/src/test/java/org/apache/helix/integration/task/TestUtil.java f599920 
> 
> Diff: https://reviews.apache.org/r/26212/diff/
> 
> 
> Testing
> -------
> 
> mvn test
> 
> 
> Thanks,
> 
> Zhen Zhang
> 
>

Re: Review Request 26212: [HELIX-519] Add integration tests to ensure that "kill-switch" for Helix tasks work as expected

Posted by Zhen Zhang <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26212/
-----------------------------------------------------------

(Updated Oct. 1, 2014, 6:06 p.m.)


Review request for helix, Kanak Biscuitwala and Shi Lu.


Bugs: 519


Repository: helix-git


Description
-------

[HELIX-519] Add integration tests to ensure that "kill-switch" for Helix tasks work as expected


Diffs (updated)
-----

  helix-core/src/main/java/org/apache/helix/task/TaskDriver.java 31d785b 
  helix-core/src/test/java/org/apache/helix/integration/task/TestTaskRebalancerStopResume.java 4e0d92a 
  helix-core/src/test/java/org/apache/helix/integration/task/TestUtil.java f599920 

Diff: https://reviews.apache.org/r/26212/diff/


Testing
-------

mvn test


Thanks,

Zhen Zhang

Re: Review Request 26212: [HELIX-519] Add integration tests to ensure that "kill-switch" for Helix tasks work as expected

Posted by Zhen Zhang <ne...@gmail.com>.

> On Oct. 1, 2014, 5 a.m., Kanak Biscuitwala wrote:
> > Ship it, but please see my comment above.

fixed. thanks!


- Zhen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26212/#review55055
-----------------------------------------------------------


On Oct. 1, 2014, 1 a.m., Zhen Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26212/
> -----------------------------------------------------------
> 
> (Updated Oct. 1, 2014, 1 a.m.)
> 
> 
> Review request for helix, Kanak Biscuitwala and Shi Lu.
> 
> 
> Bugs: 519
> 
> 
> Repository: helix-git
> 
> 
> Description
> -------
> 
> [HELIX-519] Add integration tests to ensure that "kill-switch" for Helix tasks work as expected
> 
> 
> Diffs
> -----
> 
>   helix-core/src/main/java/org/apache/helix/task/TaskDriver.java 31d785b 
>   helix-core/src/test/java/org/apache/helix/integration/task/TestTaskRebalancerStopResume.java 4e0d92a 
>   helix-core/src/test/java/org/apache/helix/integration/task/TestUtil.java f599920 
> 
> Diff: https://reviews.apache.org/r/26212/diff/
> 
> 
> Testing
> -------
> 
> mvn test
> 
> 
> Thanks,
> 
> Zhen Zhang
> 
>

Re: Review Request 26212: [HELIX-519] Add integration tests to ensure that "kill-switch" for Helix tasks work as expected

Posted by Kanak Biscuitwala <ka...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26212/#review55055
-----------------------------------------------------------

Ship it!


Ship it, but please see my comment above.

- Kanak Biscuitwala


On Sept. 30, 2014, 6 p.m., Zhen Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26212/
> -----------------------------------------------------------
> 
> (Updated Sept. 30, 2014, 6 p.m.)
> 
> 
> Review request for helix, Kanak Biscuitwala and Shi Lu.
> 
> 
> Bugs: 519
> 
> 
> Repository: helix-git
> 
> 
> Description
> -------
> 
> [HELIX-519] Add integration tests to ensure that "kill-switch" for Helix tasks work as expected
> 
> 
> Diffs
> -----
> 
>   helix-core/src/main/java/org/apache/helix/task/TaskDriver.java 31d785b 
>   helix-core/src/test/java/org/apache/helix/integration/task/TestTaskRebalancerStopResume.java 4e0d92a 
>   helix-core/src/test/java/org/apache/helix/integration/task/TestUtil.java f599920 
> 
> Diff: https://reviews.apache.org/r/26212/diff/
> 
> 
> Testing
> -------
> 
> mvn test
> 
> 
> Thanks,
> 
> Zhen Zhang
> 
>