Re: Review Request 42639: Simplify TaskHistoryPruner tie-in to Lifecycle.

[Bill Farner]

---
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.

[John Sirois]

> On Jan. 21, 2016, 7:58 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/GuavaUtils.java, line 63
> > 
> >
> > 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.

[John Sirois]

> On Jan. 21, 2016, 7:52 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java, 
> > line 169
> > 
> >
> > 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.

[Aurora ReviewBot]

---
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.

[Zameer Manji]

---
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)


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.

[Zameer Manji]

---
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
> 
>