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 2014/02/25 23:25:24 UTC

Review Request 18484: Count on task timeouts and task pruning to be idempotent, simplifying handling code.

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

Review request for Aurora, Kevin Sweeney and Suman Karumuri.

Repository: aurora

Description
-------

These async operations already took provisions to be idempotent, but still had additional code to keep track of and clean up work made irrelevant.

In this change, we explicitly lean on the fact that the operations are idempotent, and allow things like a task timeout to fire but no-op for a task that successfully made it out of a transient state.

More details on specific changes:
- HistoryPruner used to maintain a mapping from job key to task ID. Addressed TODO to use storage instead.
- Removed collection of outstanding tasks in transient states. This was useful when vetting the feature, but is no longer used in practice.
- Attempted to make HistoryPrunerTest more readable.

Diffs
-----

src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java c80c000ddb5627e9d753bd59e77231e3050470e5
src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 82b483b019639e6d1013f834ea047827cb171fa6
src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 811f68c792ce8eb3f687f249307612c90091ad59
src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java 71d6a9ed943306386dfe88074ee07f69f6ca15d1

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

Testing
-------

./gradlew build

Thanks,

Bill Farner

Re: Review Request 18484: Count on task timeouts and task pruning to be idempotent, simplifying handling code.

Posted by Kevin Sweeney <ke...@apache.org>.

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

Ship it!


Ship It!

- Kevin Sweeney


On Feb. 25, 2014, 2:25 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18484/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2014, 2:25 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Suman Karumuri.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> These async operations already took provisions to be idempotent, but still had additional code to keep track of and clean up work made irrelevant.
> 
> In this change, we explicitly lean on the fact that the operations are idempotent, and allow things like a task timeout to fire but no-op for a task that successfully made it out of a transient state.
> 
> More details on specific changes:
> - HistoryPruner used to maintain a mapping from job key to task ID.  Addressed TODO to use storage instead.
> - Removed collection of outstanding tasks in transient states.  This was useful when vetting the feature, but is no longer used in practice.
> - Attempted to make HistoryPrunerTest more readable.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java c80c000ddb5627e9d753bd59e77231e3050470e5 
>   src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 82b483b019639e6d1013f834ea047827cb171fa6 
>   src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 811f68c792ce8eb3f687f249307612c90091ad59 
>   src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java 71d6a9ed943306386dfe88074ee07f69f6ca15d1 
> 
> Diff: https://reviews.apache.org/r/18484/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 18484: Count on task timeouts and task pruning to be idempotent, simplifying handling code.

Posted by Bill Farner <wf...@apache.org>.


> On March 6, 2014, 6:56 p.m., Suman Karumuri wrote:
> > src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java, line 377
> > <https://reviews.apache.org/r/18484/diff/1/?file=503796#file503796line377>
> >
> >     This function can be replaced by Tasks.SCHEDULED_TO_JOB_KEY

Thanks, fixed.


- Bill


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


On Feb. 25, 2014, 10:25 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18484/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2014, 10:25 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Suman Karumuri.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> These async operations already took provisions to be idempotent, but still had additional code to keep track of and clean up work made irrelevant.
> 
> In this change, we explicitly lean on the fact that the operations are idempotent, and allow things like a task timeout to fire but no-op for a task that successfully made it out of a transient state.
> 
> More details on specific changes:
> - HistoryPruner used to maintain a mapping from job key to task ID.  Addressed TODO to use storage instead.
> - Removed collection of outstanding tasks in transient states.  This was useful when vetting the feature, but is no longer used in practice.
> - Attempted to make HistoryPrunerTest more readable.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java c80c000ddb5627e9d753bd59e77231e3050470e5 
>   src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 82b483b019639e6d1013f834ea047827cb171fa6 
>   src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 811f68c792ce8eb3f687f249307612c90091ad59 
>   src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java 71d6a9ed943306386dfe88074ee07f69f6ca15d1 
> 
> Diff: https://reviews.apache.org/r/18484/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 18484: Count on task timeouts and task pruning to be idempotent, simplifying handling code.

Posted by Suman Karumuri <ma...@apache.org>.

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

Ship it!



src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java
<https://reviews.apache.org/r/18484/#comment67332>

    This function can be replaced by Tasks.SCHEDULED_TO_JOB_KEY 


- Suman Karumuri


On Feb. 25, 2014, 10:25 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18484/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2014, 10:25 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Suman Karumuri.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> These async operations already took provisions to be idempotent, but still had additional code to keep track of and clean up work made irrelevant.
> 
> In this change, we explicitly lean on the fact that the operations are idempotent, and allow things like a task timeout to fire but no-op for a task that successfully made it out of a transient state.
> 
> More details on specific changes:
> - HistoryPruner used to maintain a mapping from job key to task ID.  Addressed TODO to use storage instead.
> - Removed collection of outstanding tasks in transient states.  This was useful when vetting the feature, but is no longer used in practice.
> - Attempted to make HistoryPrunerTest more readable.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java c80c000ddb5627e9d753bd59e77231e3050470e5 
>   src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 82b483b019639e6d1013f834ea047827cb171fa6 
>   src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 811f68c792ce8eb3f687f249307612c90091ad59 
>   src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java 71d6a9ed943306386dfe88074ee07f69f6ca15d1 
> 
> Diff: https://reviews.apache.org/r/18484/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 18484: Count on task timeouts and task pruning to be idempotent, simplifying handling code.

Posted by Bill Farner <wf...@apache.org>.

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


Ping?  Suman — waiting on a review from you.

- Bill Farner


On Feb. 25, 2014, 10:25 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18484/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2014, 10:25 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Suman Karumuri.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> These async operations already took provisions to be idempotent, but still had additional code to keep track of and clean up work made irrelevant.
> 
> In this change, we explicitly lean on the fact that the operations are idempotent, and allow things like a task timeout to fire but no-op for a task that successfully made it out of a transient state.
> 
> More details on specific changes:
> - HistoryPruner used to maintain a mapping from job key to task ID.  Addressed TODO to use storage instead.
> - Removed collection of outstanding tasks in transient states.  This was useful when vetting the feature, but is no longer used in practice.
> - Attempted to make HistoryPrunerTest more readable.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java c80c000ddb5627e9d753bd59e77231e3050470e5 
>   src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 82b483b019639e6d1013f834ea047827cb171fa6 
>   src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 811f68c792ce8eb3f687f249307612c90091ad59 
>   src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java 71d6a9ed943306386dfe88074ee07f69f6ca15d1 
> 
> Diff: https://reviews.apache.org/r/18484/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 18484: Count on task timeouts and task pruning to be idempotent, simplifying handling code.

Posted by Bill Farner <wf...@apache.org>.

(Updated March 7, 2014, 12:22 a.m.)

Review request for Aurora, Kevin Sweeney and Suman Karumuri.

Repository: aurora

Description
-------

These async operations already took provisions to be idempotent, but still had additional code to keep track of and clean up work made irrelevant.

Diffs (updated)
-----

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

Testing
-------

./gradlew build

Thanks,

Bill Farner