You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by John Sirois <js...@apache.org> on 2016/01/22 03:47:54 UTC
Review Request 42639: Simplify TaskHistoryPruner tie-in to Lifecycle.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42639/
-----------------------------------------------------------
Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
Bugs: AURORA-1582
https://issues.apache.org/jira/browse/AURORA-1582
Repository: aurora
Description
-------
This eliminates processing all futures to find the 1st failed one in
favor of directly signalling a Service failure when a unit of async work
fails.
src/main/java/org/apache/aurora/GuavaUtils.java | 18 ++++++++++++++++
src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java | 56 ++++++++++++++++++--------------------------------
src/test/java/org/apache/aurora/LifecycleShutdownListenerTest.java | 17 ++-------------
3 files changed, 40 insertions(+), 51 deletions(-)
Diffs
-----
src/main/java/org/apache/aurora/GuavaUtils.java 8c2ab5720b808e5eae4c6b93fea8b787839b8dbe
src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 2d4c58eaa930c561446320a77a559cd926318e8a
src/test/java/org/apache/aurora/LifecycleShutdownListenerTest.java 8d19c04a96fccdd9594ecf783fc797c2b76140c7
Diff: https://reviews.apache.org/r/42639/diff/
Testing
-------
Locally green: `./gradlew -P build`.
Thanks,
John Sirois
Re: Review Request 42639: Simplify TaskHistoryPruner tie-in to
Lifecycle.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42639/#review115777
-----------------------------------------------------------
Ship it!
Ship It!
- Maxim Khutornenko
On Jan. 22, 2016, 2:47 a.m., John Sirois wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42639/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 2:47 a.m.)
>
>
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
>
>
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This eliminates processing all futures to find the 1st failed one in
> favor of directly signalling a Service failure when a unit of async work
> fails.
>
> src/main/java/org/apache/aurora/GuavaUtils.java | 18 ++++++++++++++++
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java | 56 ++++++++++++++++++--------------------------------
> src/test/java/org/apache/aurora/LifecycleShutdownListenerTest.java | 17 ++-------------
> 3 files changed, 40 insertions(+), 51 deletions(-)
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/GuavaUtils.java 8c2ab5720b808e5eae4c6b93fea8b787839b8dbe
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 2d4c58eaa930c561446320a77a559cd926318e8a
> src/test/java/org/apache/aurora/LifecycleShutdownListenerTest.java 8d19c04a96fccdd9594ecf783fc797c2b76140c7
>
> Diff: https://reviews.apache.org/r/42639/diff/
>
>
> Testing
> -------
>
> Locally green: `./gradlew -P build`.
>
>
> Thanks,
>
> John Sirois
>
>
Re: Review Request 42639: Simplify TaskHistoryPruner tie-in to
Lifecycle.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42639/#review115911
-----------------------------------------------------------
Ship it!
Ship It!
- Bill Farner
On Jan. 21, 2016, 6:47 p.m., John Sirois wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42639/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2016, 6:47 p.m.)
>
>
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
>
>
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This eliminates processing all futures to find the 1st failed one in
> favor of directly signalling a Service failure when a unit of async work
> fails.
>
> src/main/java/org/apache/aurora/GuavaUtils.java | 18 ++++++++++++++++
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java | 56 ++++++++++++++++++--------------------------------
> src/test/java/org/apache/aurora/LifecycleShutdownListenerTest.java | 17 ++-------------
> 3 files changed, 40 insertions(+), 51 deletions(-)
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/GuavaUtils.java 8c2ab5720b808e5eae4c6b93fea8b787839b8dbe
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 2d4c58eaa930c561446320a77a559cd926318e8a
> src/test/java/org/apache/aurora/LifecycleShutdownListenerTest.java 8d19c04a96fccdd9594ecf783fc797c2b76140c7
>
> Diff: https://reviews.apache.org/r/42639/diff/
>
>
> Testing
> -------
>
> Locally green: `./gradlew -P build`.
>
>
> Thanks,
>
> John Sirois
>
>
Re: Review Request 42639: Simplify TaskHistoryPruner tie-in to
Lifecycle.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42639/#review115774
-----------------------------------------------------------
Ship it!
Master (c89fecb) 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 Jan. 22, 2016, 2:47 a.m., John Sirois wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42639/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 2:47 a.m.)
>
>
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
>
>
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This eliminates processing all futures to find the 1st failed one in
> favor of directly signalling a Service failure when a unit of async work
> fails.
>
> src/main/java/org/apache/aurora/GuavaUtils.java | 18 ++++++++++++++++
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java | 56 ++++++++++++++++++--------------------------------
> src/test/java/org/apache/aurora/LifecycleShutdownListenerTest.java | 17 ++-------------
> 3 files changed, 40 insertions(+), 51 deletions(-)
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/GuavaUtils.java 8c2ab5720b808e5eae4c6b93fea8b787839b8dbe
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 2d4c58eaa930c561446320a77a559cd926318e8a
> src/test/java/org/apache/aurora/LifecycleShutdownListenerTest.java 8d19c04a96fccdd9594ecf783fc797c2b76140c7
>
> Diff: https://reviews.apache.org/r/42639/diff/
>
>
> Testing
> -------
>
> Locally green: `./gradlew -P build`.
>
>
> Thanks,
>
> John Sirois
>
>
Re: Review Request 42639: Simplify TaskHistoryPruner tie-in to
Lifecycle.
Posted by John Sirois <js...@apache.org>.
> On Jan. 21, 2016, 7:58 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/GuavaUtils.java, line 63
> > <https://reviews.apache.org/r/42639/diff/1/?file=1205277#file1205277line63>
> >
> > Would not AbstractIdleService suffice your needs here? http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/util/concurrent/AbstractIdleService.html?
See above - unfortunately no. It does not expose `notifyFailed`.
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42639/#review115770
-----------------------------------------------------------
On Jan. 21, 2016, 7:47 p.m., John Sirois wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42639/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2016, 7:47 p.m.)
>
>
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
>
>
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This eliminates processing all futures to find the 1st failed one in
> favor of directly signalling a Service failure when a unit of async work
> fails.
>
> src/main/java/org/apache/aurora/GuavaUtils.java | 18 ++++++++++++++++
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java | 56 ++++++++++++++++++--------------------------------
> src/test/java/org/apache/aurora/LifecycleShutdownListenerTest.java | 17 ++-------------
> 3 files changed, 40 insertions(+), 51 deletions(-)
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/GuavaUtils.java 8c2ab5720b808e5eae4c6b93fea8b787839b8dbe
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 2d4c58eaa930c561446320a77a559cd926318e8a
> src/test/java/org/apache/aurora/LifecycleShutdownListenerTest.java 8d19c04a96fccdd9594ecf783fc797c2b76140c7
>
> Diff: https://reviews.apache.org/r/42639/diff/
>
>
> Testing
> -------
>
> Locally green: `./gradlew -P build`.
>
>
> Thanks,
>
> John Sirois
>
>
Re: Review Request 42639: Simplify TaskHistoryPruner tie-in to
Lifecycle.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42639/#review115770
-----------------------------------------------------------
src/main/java/org/apache/aurora/GuavaUtils.java (line 63)
<https://reviews.apache.org/r/42639/#comment176833>
Would not AbstractIdleService suffice your needs here? http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/util/concurrent/AbstractIdleService.html?
- Maxim Khutornenko
On Jan. 22, 2016, 2:47 a.m., John Sirois wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42639/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 2:47 a.m.)
>
>
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
>
>
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This eliminates processing all futures to find the 1st failed one in
> favor of directly signalling a Service failure when a unit of async work
> fails.
>
> src/main/java/org/apache/aurora/GuavaUtils.java | 18 ++++++++++++++++
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java | 56 ++++++++++++++++++--------------------------------
> src/test/java/org/apache/aurora/LifecycleShutdownListenerTest.java | 17 ++-------------
> 3 files changed, 40 insertions(+), 51 deletions(-)
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/GuavaUtils.java 8c2ab5720b808e5eae4c6b93fea8b787839b8dbe
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 2d4c58eaa930c561446320a77a559cd926318e8a
> src/test/java/org/apache/aurora/LifecycleShutdownListenerTest.java 8d19c04a96fccdd9594ecf783fc797c2b76140c7
>
> Diff: https://reviews.apache.org/r/42639/diff/
>
>
> Testing
> -------
>
> Locally green: `./gradlew -P build`.
>
>
> Thanks,
>
> John Sirois
>
>
Re: Review Request 42639: Simplify TaskHistoryPruner tie-in to
Lifecycle.
Posted by John Sirois <js...@apache.org>.
> On Jan. 21, 2016, 7:52 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java, line 169
> > <https://reviews.apache.org/r/42639/diff/1/?file=1205278#file1205278line169>
> >
> > Is it safe to call this method from multiple threads? Nothing in the Guava documentation indicates this is the case.
By direct inspection - yes. Wider inspection finds all state handling in all com.google.common.util.concurrent Service implementations uses Guard and Monitor.
My path here was AbstractIdleService which unfortunately implements Service directly, but uses an internal AbstractService delegate that uses `notifyFailed` both in `doStart` and `doStop` and in both cases in new Threads by default.
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42639/#review115768
-----------------------------------------------------------
On Jan. 21, 2016, 7:47 p.m., John Sirois wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42639/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2016, 7:47 p.m.)
>
>
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
>
>
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This eliminates processing all futures to find the 1st failed one in
> favor of directly signalling a Service failure when a unit of async work
> fails.
>
> src/main/java/org/apache/aurora/GuavaUtils.java | 18 ++++++++++++++++
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java | 56 ++++++++++++++++++--------------------------------
> src/test/java/org/apache/aurora/LifecycleShutdownListenerTest.java | 17 ++-------------
> 3 files changed, 40 insertions(+), 51 deletions(-)
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/GuavaUtils.java 8c2ab5720b808e5eae4c6b93fea8b787839b8dbe
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 2d4c58eaa930c561446320a77a559cd926318e8a
> src/test/java/org/apache/aurora/LifecycleShutdownListenerTest.java 8d19c04a96fccdd9594ecf783fc797c2b76140c7
>
> Diff: https://reviews.apache.org/r/42639/diff/
>
>
> Testing
> -------
>
> Locally green: `./gradlew -P build`.
>
>
> Thanks,
>
> John Sirois
>
>
Re: Review Request 42639: Simplify TaskHistoryPruner tie-in to
Lifecycle.
Posted by Zameer Manji <zm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42639/#review115768
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java (line 146)
<https://reviews.apache.org/r/42639/#comment176831>
Is it safe to call this method from multiple threads? Nothing in the Guava documentation indicates this is the case.
- Zameer Manji
On Jan. 21, 2016, 6:47 p.m., John Sirois wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42639/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2016, 6:47 p.m.)
>
>
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
>
>
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This eliminates processing all futures to find the 1st failed one in
> favor of directly signalling a Service failure when a unit of async work
> fails.
>
> src/main/java/org/apache/aurora/GuavaUtils.java | 18 ++++++++++++++++
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java | 56 ++++++++++++++++++--------------------------------
> src/test/java/org/apache/aurora/LifecycleShutdownListenerTest.java | 17 ++-------------
> 3 files changed, 40 insertions(+), 51 deletions(-)
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/GuavaUtils.java 8c2ab5720b808e5eae4c6b93fea8b787839b8dbe
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 2d4c58eaa930c561446320a77a559cd926318e8a
> src/test/java/org/apache/aurora/LifecycleShutdownListenerTest.java 8d19c04a96fccdd9594ecf783fc797c2b76140c7
>
> Diff: https://reviews.apache.org/r/42639/diff/
>
>
> Testing
> -------
>
> Locally green: `./gradlew -P build`.
>
>
> Thanks,
>
> John Sirois
>
>
Re: Review Request 42639: Simplify TaskHistoryPruner tie-in to
Lifecycle.
Posted by Zameer Manji <zm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42639/#review115782
-----------------------------------------------------------
Ship it!
Ship It!
- Zameer Manji
On Jan. 21, 2016, 6:47 p.m., John Sirois wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42639/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2016, 6:47 p.m.)
>
>
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
>
>
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This eliminates processing all futures to find the 1st failed one in
> favor of directly signalling a Service failure when a unit of async work
> fails.
>
> src/main/java/org/apache/aurora/GuavaUtils.java | 18 ++++++++++++++++
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java | 56 ++++++++++++++++++--------------------------------
> src/test/java/org/apache/aurora/LifecycleShutdownListenerTest.java | 17 ++-------------
> 3 files changed, 40 insertions(+), 51 deletions(-)
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/GuavaUtils.java 8c2ab5720b808e5eae4c6b93fea8b787839b8dbe
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 2d4c58eaa930c561446320a77a559cd926318e8a
> src/test/java/org/apache/aurora/LifecycleShutdownListenerTest.java 8d19c04a96fccdd9594ecf783fc797c2b76140c7
>
> Diff: https://reviews.apache.org/r/42639/diff/
>
>
> Testing
> -------
>
> Locally green: `./gradlew -P build`.
>
>
> Thanks,
>
> John Sirois
>
>