You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Bill Farner <wf...@apache.org> on 2015/07/23 02:07:13 UTC
Review Request 36710: Add an executor service decorator that gates
async operations.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36710/
-----------------------------------------------------------
Review request for Aurora, Kevin Sweeney and Zameer Manji.
Bugs: AURORA-1395
https://issues.apache.org/jira/browse/AURORA-1395
Repository: aurora
Description
-------
This will be used by the rest of the system to manage deferment of work until a transaction has completed.
TODO(wfarner): I need to document the newly-added interfaces, please assume i will add that shortly.
Diffs
-----
src/main/java/org/apache/aurora/scheduler/async/DeferredWork.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/async/FlushableWorkQueue.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/async/GatedScheduledExecutorService.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/async/TimeDeferredWork.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java c85979dedd0ef2c515453a33c9a36d52865eb548
src/test/java/org/apache/aurora/scheduler/async/GatedScheduledExecutorServiceTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/36710/diff/
Testing
-------
Thanks,
Bill Farner
Re: Review Request 36710: Add an executor service decorator that
gates async operations.
Posted by Kevin Sweeney <ke...@apache.org>.
> On July 22, 2015, 5:11 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/async/GatedScheduledExecutorService.java, line 36
> > <https://reviews.apache.org/r/36710/diff/1/?file=1019150#file1019150line36>
> >
> > Consider extending http://docs.guava-libraries.googlecode.com/git-history/release/javadoc/com/google/common/util/concurrent/ForwardingListeningExecutorService.html instead, for the reasons outlined in https://code.google.com/p/guava-libraries/wiki/ListenableFutureExplained
>
> Bill Farner wrote:
> As you may not have noticed, i am returning `null` from all Future-providing methods since we never actually use the return values (and it would add complexity to support Futures). If we go this route, the richness of listenable futures is moot.
Is maintaining the ExecutorService interface necessary for some call sites you intend to replace? Would it be reasonable make this a new interface without the partial implementation?
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36710/#review92685
-----------------------------------------------------------
On July 22, 2015, 5:07 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36710/
> -----------------------------------------------------------
>
> (Updated July 22, 2015, 5:07 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Zameer Manji.
>
>
> Bugs: AURORA-1395
> https://issues.apache.org/jira/browse/AURORA-1395
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This will be used by the rest of the system to manage deferment of work until a transaction has completed.
>
> TODO(wfarner): I need to document the newly-added interfaces, please assume i will add that shortly.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/DeferredWork.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/async/FlushableWorkQueue.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/async/GatedScheduledExecutorService.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/async/TimeDeferredWork.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java c85979dedd0ef2c515453a33c9a36d52865eb548
> src/test/java/org/apache/aurora/scheduler/async/GatedScheduledExecutorServiceTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/36710/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 36710: Add an executor service decorator that
gates async operations.
Posted by Bill Farner <wf...@apache.org>.
> On July 23, 2015, 12:11 a.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/async/GatedScheduledExecutorService.java, line 36
> > <https://reviews.apache.org/r/36710/diff/1/?file=1019150#file1019150line36>
> >
> > Consider extending http://docs.guava-libraries.googlecode.com/git-history/release/javadoc/com/google/common/util/concurrent/ForwardingListeningExecutorService.html instead, for the reasons outlined in https://code.google.com/p/guava-libraries/wiki/ListenableFutureExplained
>
> Bill Farner wrote:
> As you may not have noticed, i am returning `null` from all Future-providing methods since we never actually use the return values (and it would add complexity to support Futures). If we go this route, the richness of listenable futures is moot.
>
> Kevin Sweeney wrote:
> Is maintaining the ExecutorService interface necessary for some call sites you intend to replace? Would it be reasonable make this a new interface without the partial implementation?
Thanks for pushing - i started down that path originally, but convinced myself that we needed to implement at least `ExecutorService` for `AsyncEventBus` purposes, but that only requires `Executor`. This will slim things down considerably, will post a new diff shortly.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36710/#review92685
-----------------------------------------------------------
On July 23, 2015, 12:07 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36710/
> -----------------------------------------------------------
>
> (Updated July 23, 2015, 12:07 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Zameer Manji.
>
>
> Bugs: AURORA-1395
> https://issues.apache.org/jira/browse/AURORA-1395
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This will be used by the rest of the system to manage deferment of work until a transaction has completed.
>
> TODO(wfarner): I need to document the newly-added interfaces, please assume i will add that shortly.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/DeferredWork.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/async/FlushableWorkQueue.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/async/GatedScheduledExecutorService.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/async/TimeDeferredWork.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java c85979dedd0ef2c515453a33c9a36d52865eb548
> src/test/java/org/apache/aurora/scheduler/async/GatedScheduledExecutorServiceTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/36710/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 36710: Add an executor service decorator that
gates async operations.
Posted by Bill Farner <wf...@apache.org>.
> On July 23, 2015, 12:11 a.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/async/GatedScheduledExecutorService.java, line 36
> > <https://reviews.apache.org/r/36710/diff/1/?file=1019150#file1019150line36>
> >
> > Consider extending http://docs.guava-libraries.googlecode.com/git-history/release/javadoc/com/google/common/util/concurrent/ForwardingListeningExecutorService.html instead, for the reasons outlined in https://code.google.com/p/guava-libraries/wiki/ListenableFutureExplained
As you may not have noticed, i am returning `null` from all Future-providing methods since we never actually use the return values (and it would add complexity to support Futures). If we go this route, the richness of listenable futures is moot.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36710/#review92685
-----------------------------------------------------------
On July 23, 2015, 12:07 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36710/
> -----------------------------------------------------------
>
> (Updated July 23, 2015, 12:07 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Zameer Manji.
>
>
> Bugs: AURORA-1395
> https://issues.apache.org/jira/browse/AURORA-1395
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This will be used by the rest of the system to manage deferment of work until a transaction has completed.
>
> TODO(wfarner): I need to document the newly-added interfaces, please assume i will add that shortly.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/DeferredWork.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/async/FlushableWorkQueue.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/async/GatedScheduledExecutorService.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/async/TimeDeferredWork.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java c85979dedd0ef2c515453a33c9a36d52865eb548
> src/test/java/org/apache/aurora/scheduler/async/GatedScheduledExecutorServiceTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/36710/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 36710: Add an executor service decorator that
gates async operations.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36710/#review92685
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/async/GatedScheduledExecutorService.java (line 36)
<https://reviews.apache.org/r/36710/#comment146913>
Consider extending http://docs.guava-libraries.googlecode.com/git-history/release/javadoc/com/google/common/util/concurrent/ForwardingListeningExecutorService.html instead, for the reasons outlined in https://code.google.com/p/guava-libraries/wiki/ListenableFutureExplained
- Kevin Sweeney
On July 22, 2015, 5:07 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36710/
> -----------------------------------------------------------
>
> (Updated July 22, 2015, 5:07 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Zameer Manji.
>
>
> Bugs: AURORA-1395
> https://issues.apache.org/jira/browse/AURORA-1395
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This will be used by the rest of the system to manage deferment of work until a transaction has completed.
>
> TODO(wfarner): I need to document the newly-added interfaces, please assume i will add that shortly.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/DeferredWork.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/async/FlushableWorkQueue.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/async/GatedScheduledExecutorService.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/async/TimeDeferredWork.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java c85979dedd0ef2c515453a33c9a36d52865eb548
> src/test/java/org/apache/aurora/scheduler/async/GatedScheduledExecutorServiceTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/36710/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 36710: Add an executor service decorator that
gates async operations.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36710/#review92689
-----------------------------------------------------------
Ship it!
Master (38c2e76) 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 July 23, 2015, 12:07 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36710/
> -----------------------------------------------------------
>
> (Updated July 23, 2015, 12:07 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Zameer Manji.
>
>
> Bugs: AURORA-1395
> https://issues.apache.org/jira/browse/AURORA-1395
>
>
> Repository: aurora
>
>
> Description
> -------
>
> This will be used by the rest of the system to manage deferment of work until a transaction has completed.
>
> TODO(wfarner): I need to document the newly-added interfaces, please assume i will add that shortly.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/DeferredWork.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/async/FlushableWorkQueue.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/async/GatedScheduledExecutorService.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/async/TimeDeferredWork.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java c85979dedd0ef2c515453a33c9a36d52865eb548
> src/test/java/org/apache/aurora/scheduler/async/GatedScheduledExecutorServiceTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/36710/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 36710: Add an executor service decorator that
gates async operations.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36710/#review92772
-----------------------------------------------------------
Ship it!
Master (96b56b8) 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 July 23, 2015, 5:20 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36710/
> -----------------------------------------------------------
>
> (Updated July 23, 2015, 5:20 p.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> Bugs: AURORA-1395
> https://issues.apache.org/jira/browse/AURORA-1395
>
>
> Repository: aurora
>
>
> Description
> -------
>
> In the current state of the scheduler, we start async work in the context of a transaction. As we have observed in the linked ticket, this work can read data that is inconsistent with the transaction context if it starts before the transaction completes. The purpose of this utility is to let async work queue until the originating transaction completes.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/DelayExecutor.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/async/FlushableWorkQueue.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/async/GatedDelayExecutor.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java c85979dedd0ef2c515453a33c9a36d52865eb548
> src/test/java/org/apache/aurora/scheduler/async/GatedDelayExecutorTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/36710/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 36710: Add an executor service decorator that
gates async operations.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36710/#review93000
-----------------------------------------------------------
Ship it!
Ship It!
- Kevin Sweeney
On July 23, 2015, 10:20 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36710/
> -----------------------------------------------------------
>
> (Updated July 23, 2015, 10:20 a.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> Bugs: AURORA-1395
> https://issues.apache.org/jira/browse/AURORA-1395
>
>
> Repository: aurora
>
>
> Description
> -------
>
> In the current state of the scheduler, we start async work in the context of a transaction. As we have observed in the linked ticket, this work can read data that is inconsistent with the transaction context if it starts before the transaction completes. The purpose of this utility is to let async work queue until the originating transaction completes.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/DelayExecutor.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/async/FlushableWorkQueue.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/async/GatedDelayExecutor.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java c85979dedd0ef2c515453a33c9a36d52865eb548
> src/test/java/org/apache/aurora/scheduler/async/GatedDelayExecutorTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/36710/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 36710: Add an executor service decorator that
gates async operations.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36710/
-----------------------------------------------------------
(Updated July 23, 2015, 5:20 p.m.)
Review request for Aurora and Kevin Sweeney.
Changes
-------
Slimmed implemented interface to `Executor`.
Bugs: AURORA-1395
https://issues.apache.org/jira/browse/AURORA-1395
Repository: aurora
Description (updated)
-------
In the current state of the scheduler, we start async work in the context of a transaction. As we have observed in the linked ticket, this work can read data that is inconsistent with the transaction context if it starts before the transaction completes. The purpose of this utility is to let async work queue until the originating transaction completes.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/async/DelayExecutor.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/async/FlushableWorkQueue.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/async/GatedDelayExecutor.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java c85979dedd0ef2c515453a33c9a36d52865eb548
src/test/java/org/apache/aurora/scheduler/async/GatedDelayExecutorTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/36710/diff/
Testing
-------
Thanks,
Bill Farner