You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Jordan Ly <jo...@gmail.com> on 2017/09/27 20:25:56 UTC
Review Request 62626: Added additional stop() to prevent errors in
run() to stop shutdown in SchedulerMain
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62626/
-----------------------------------------------------------
Review request for Aurora, David McLaughlin and Bill Farner.
Bugs: AURORA-1950
https://issues.apache.org/jira/browse/AURORA-1950
Repository: aurora
Description
-------
Ensure that `SchedulerMain.run()` calls stop in the case of exceptions. This prevents the Scheduler from being transitioned to DEAD state, but not actually stopping it's services.
See the attached ticket for an example of issue happening.
Diffs
-----
src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3fbe99c4f1af1f11403bf08155bb4be028132e38
src/test/java/org/apache/aurora/scheduler/SchedulerLifecycleTest.java 70479ef8be324bf2e382b0d2ad594c2eb33c931c
Diff: https://reviews.apache.org/r/62626/diff/1/
Testing
-------
Added an additional unit test for prepare() failing in `SchedulerLifecycle.java`.
./gradlew test
./build-support/jenkin/build.sh
Thanks,
Jordan Ly
Re: Review Request 62626: Added additional stop() to prevent errors
in run() to stop shutdown in SchedulerMain
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62626/#review186478
-----------------------------------------------------------
Ship it!
Master (abd6fad) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Sept. 27, 2017, 8:25 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62626/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2017, 8:25 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-1950
> https://issues.apache.org/jira/browse/AURORA-1950
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Ensure that `SchedulerMain.run()` calls stop in the case of exceptions. This prevents the Scheduler from being transitioned to DEAD state, but not actually stopping it's services.
>
> See the attached ticket for an example of issue happening.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3fbe99c4f1af1f11403bf08155bb4be028132e38
> src/test/java/org/apache/aurora/scheduler/SchedulerLifecycleTest.java 70479ef8be324bf2e382b0d2ad594c2eb33c931c
>
>
> Diff: https://reviews.apache.org/r/62626/diff/1/
>
>
> Testing
> -------
>
> Added an additional unit test for prepare() failing in `SchedulerLifecycle.java`.
>
> ./gradlew test
> ./build-support/jenkin/build.sh
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62626: Added additional stop() to prevent errors
in run() to stop shutdown in SchedulerMain
Posted by Jordan Ly <jo...@gmail.com>.
> On Sept. 27, 2017, 8:33 p.m., Reza Motamedi wrote:
> > src/test/java/org/apache/aurora/scheduler/SchedulerLifecycleTest.java
> > Line 109 (original), 109 (patched)
> > <https://reviews.apache.org/r/62626/diff/1/?file=1837865#file1837865line109>
> >
> > Is just testing shutdown of the leader? Should we have separate tests the followers then?
I believe this is just testing the expectations around shutdowns in `SchedulerLifecycle.java`. This method specifically expects everything in `shutdown()` as well as that the leader leaves the control. Followers should just be able to use `shutdown()`.
- Jordan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62626/#review186473
-----------------------------------------------------------
On Sept. 27, 2017, 8:25 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62626/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2017, 8:25 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-1950
> https://issues.apache.org/jira/browse/AURORA-1950
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Ensure that `SchedulerMain.run()` calls stop in the case of exceptions. This prevents the Scheduler from being transitioned to DEAD state, but not actually stopping it's services.
>
> See the attached ticket for an example of issue happening.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3fbe99c4f1af1f11403bf08155bb4be028132e38
> src/test/java/org/apache/aurora/scheduler/SchedulerLifecycleTest.java 70479ef8be324bf2e382b0d2ad594c2eb33c931c
>
>
> Diff: https://reviews.apache.org/r/62626/diff/1/
>
>
> Testing
> -------
>
> Added an additional unit test for prepare() failing in `SchedulerLifecycle.java`.
>
> ./gradlew test
> ./build-support/jenkin/build.sh
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62626: Added additional stop() to prevent errors
in run() to stop shutdown in SchedulerMain
Posted by Reza Motamedi <re...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62626/#review186473
-----------------------------------------------------------
src/test/java/org/apache/aurora/scheduler/SchedulerLifecycleTest.java
Line 109 (original), 109 (patched)
<https://reviews.apache.org/r/62626/#comment263029>
Is just testing shutdown of the leader? Should we have separate tests the followers then?
- Reza Motamedi
On Sept. 27, 2017, 8:25 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62626/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2017, 8:25 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-1950
> https://issues.apache.org/jira/browse/AURORA-1950
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Ensure that `SchedulerMain.run()` calls stop in the case of exceptions. This prevents the Scheduler from being transitioned to DEAD state, but not actually stopping it's services.
>
> See the attached ticket for an example of issue happening.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3fbe99c4f1af1f11403bf08155bb4be028132e38
> src/test/java/org/apache/aurora/scheduler/SchedulerLifecycleTest.java 70479ef8be324bf2e382b0d2ad594c2eb33c931c
>
>
> Diff: https://reviews.apache.org/r/62626/diff/1/
>
>
> Testing
> -------
>
> Added an additional unit test for prepare() failing in `SchedulerLifecycle.java`.
>
> ./gradlew test
> ./build-support/jenkin/build.sh
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62626: Added additional stop() to prevent errors
in run() to stop shutdown in SchedulerMain
Posted by Reza Motamedi <re...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62626/#review186618
-----------------------------------------------------------
Ship it!
Ship It!
- Reza Motamedi
On Sept. 27, 2017, 8:25 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62626/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2017, 8:25 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-1950
> https://issues.apache.org/jira/browse/AURORA-1950
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Ensure that `SchedulerMain.run()` calls stop in the case of exceptions. This prevents the Scheduler from being transitioned to DEAD state, but not actually stopping it's services.
>
> See the attached ticket for an example of issue happening.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3fbe99c4f1af1f11403bf08155bb4be028132e38
> src/test/java/org/apache/aurora/scheduler/SchedulerLifecycleTest.java 70479ef8be324bf2e382b0d2ad594c2eb33c931c
>
>
> Diff: https://reviews.apache.org/r/62626/diff/1/
>
>
> Testing
> -------
>
> Added an additional unit test for prepare() failing in `SchedulerLifecycle.java`.
>
> ./gradlew test
> ./build-support/jenkin/build.sh
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62626: Added additional stop() to prevent errors
in run() to stop shutdown in SchedulerMain
Posted by Stephan Erb <se...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62626/#review186497
-----------------------------------------------------------
Ship it!
Ship It!
- Stephan Erb
On Sept. 27, 2017, 10:25 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62626/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2017, 10:25 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-1950
> https://issues.apache.org/jira/browse/AURORA-1950
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Ensure that `SchedulerMain.run()` calls stop in the case of exceptions. This prevents the Scheduler from being transitioned to DEAD state, but not actually stopping it's services.
>
> See the attached ticket for an example of issue happening.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3fbe99c4f1af1f11403bf08155bb4be028132e38
> src/test/java/org/apache/aurora/scheduler/SchedulerLifecycleTest.java 70479ef8be324bf2e382b0d2ad594c2eb33c931c
>
>
> Diff: https://reviews.apache.org/r/62626/diff/1/
>
>
> Testing
> -------
>
> Added an additional unit test for prepare() failing in `SchedulerLifecycle.java`.
>
> ./gradlew test
> ./build-support/jenkin/build.sh
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62626: Added additional stop() to prevent errors
in run() to stop shutdown in SchedulerMain
Posted by Jordan Ly <jo...@gmail.com>.
> On Sept. 27, 2017, 9:45 p.m., Santhosh Kumar Shanmugham wrote:
> > Wonder if this will fix the issue with shutting down the Webhooks AsyncHttpClient as well?
This should just always call the shutdown sequence when `run()` ends, (ie. `stop()` is always called in the case of errors and etc). Looking at the Webhooks patch, this patch will unfortunately not help. We will need to convert Webhooks into a `Service`, write a suitable `shutdown()` method that closes the AsyncHttpClient, and register the service to be initiated on startup (or leader election, not sure how it works).
- Jordan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62626/#review186488
-----------------------------------------------------------
On Sept. 27, 2017, 8:25 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62626/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2017, 8:25 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-1950
> https://issues.apache.org/jira/browse/AURORA-1950
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Ensure that `SchedulerMain.run()` calls stop in the case of exceptions. This prevents the Scheduler from being transitioned to DEAD state, but not actually stopping it's services.
>
> See the attached ticket for an example of issue happening.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3fbe99c4f1af1f11403bf08155bb4be028132e38
> src/test/java/org/apache/aurora/scheduler/SchedulerLifecycleTest.java 70479ef8be324bf2e382b0d2ad594c2eb33c931c
>
>
> Diff: https://reviews.apache.org/r/62626/diff/1/
>
>
> Testing
> -------
>
> Added an additional unit test for prepare() failing in `SchedulerLifecycle.java`.
>
> ./gradlew test
> ./build-support/jenkin/build.sh
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62626: Added additional stop() to prevent errors
in run() to stop shutdown in SchedulerMain
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62626/#review186488
-----------------------------------------------------------
Ship it!
Wonder if this will fix the issue with shutting down the Webhooks AsyncHttpClient as well?
- Santhosh Kumar Shanmugham
On Sept. 27, 2017, 1:25 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62626/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2017, 1:25 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-1950
> https://issues.apache.org/jira/browse/AURORA-1950
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Ensure that `SchedulerMain.run()` calls stop in the case of exceptions. This prevents the Scheduler from being transitioned to DEAD state, but not actually stopping it's services.
>
> See the attached ticket for an example of issue happening.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3fbe99c4f1af1f11403bf08155bb4be028132e38
> src/test/java/org/apache/aurora/scheduler/SchedulerLifecycleTest.java 70479ef8be324bf2e382b0d2ad594c2eb33c931c
>
>
> Diff: https://reviews.apache.org/r/62626/diff/1/
>
>
> Testing
> -------
>
> Added an additional unit test for prepare() failing in `SchedulerLifecycle.java`.
>
> ./gradlew test
> ./build-support/jenkin/build.sh
>
>
> Thanks,
>
> Jordan Ly
>
>
Re: Review Request 62626: Added additional stop() to prevent errors
in run() to stop shutdown in SchedulerMain
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62626/#review186518
-----------------------------------------------------------
Ship it!
Ship It!
- Bill Farner
On Sept. 27, 2017, 1:25 p.m., Jordan Ly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62626/
> -----------------------------------------------------------
>
> (Updated Sept. 27, 2017, 1:25 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-1950
> https://issues.apache.org/jira/browse/AURORA-1950
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Ensure that `SchedulerMain.run()` calls stop in the case of exceptions. This prevents the Scheduler from being transitioned to DEAD state, but not actually stopping it's services.
>
> See the attached ticket for an example of issue happening.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 3fbe99c4f1af1f11403bf08155bb4be028132e38
> src/test/java/org/apache/aurora/scheduler/SchedulerLifecycleTest.java 70479ef8be324bf2e382b0d2ad594c2eb33c931c
>
>
> Diff: https://reviews.apache.org/r/62626/diff/1/
>
>
> Testing
> -------
>
> Added an additional unit test for prepare() failing in `SchedulerLifecycle.java`.
>
> ./gradlew test
> ./build-support/jenkin/build.sh
>
>
> Thanks,
>
> Jordan Ly
>
>