Re: Review Request 19243: Adding min retention interval for task history.

[Kevin Sweeney]

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

Ship it!


Ship It!

- Kevin Sweeney


On March 19, 2014, 2:38 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19243/
> ---
> 
> (Updated March 19, 2014, 2:38 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-263
> https://issues.apache.org/jira/browse/AURORA-263
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding min retention interval for task history.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java 
> be67e7d1f93584542a645ada4499e26ab9bb1fb4 
>   src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java 
> 5bf9838e56f074fc5e66ace2a9c4e7bc50d1480f 
>   src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 
> f7c9e5ed408b44e93df8cfff9c9beeb2f717b2d2 
> 
> Diff: https://reviews.apache.org/r/19243/diff/
> 
> 
> Testing
> ---
> 
> gradle build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 19243: Adding min retention interval for task history.

[Bill Farner]

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

Ship it!


Ship It!

- Bill Farner


On March 19, 2014, 9:38 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19243/
> ---
> 
> (Updated March 19, 2014, 9:38 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-263
> https://issues.apache.org/jira/browse/AURORA-263
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding min retention interval for task history.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java 
> be67e7d1f93584542a645ada4499e26ab9bb1fb4 
>   src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java 
> 5bf9838e56f074fc5e66ace2a9c4e7bc50d1480f 
>   src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 
> f7c9e5ed408b44e93df8cfff9c9beeb2f717b2d2 
> 
> Diff: https://reviews.apache.org/r/19243/diff/
> 
> 
> Testing
> ---
> 
> gradle build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 19243: Adding min retention interval for task history.

[Maxim Khutornenko]

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

(Updated March 19, 2014, 9:38 p.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

CR comments.


Bugs: AURORA-263
https://issues.apache.org/jira/browse/AURORA-263


Repository: aurora


Description
---

Adding min retention interval for task history.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java 
be67e7d1f93584542a645ada4499e26ab9bb1fb4 
  src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java 
5bf9838e56f074fc5e66ace2a9c4e7bc50d1480f 
  src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 
f7c9e5ed408b44e93df8cfff9c9beeb2f717b2d2 

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


Testing
---

gradle build


Thanks,

Maxim Khutornenko

Re: Review Request 19243: Adding min retention interval for task history.

[Maxim Khutornenko]

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



src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java


Sure, that might result in more memory consumption but should not matter 
much given our throttling scheduling restrictions. Done.



src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java


Done.



src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java


The way I see it there are 2 independent pruning actions:
1. Scheduled individual task pruning via executor.schedule();
2. Opportunistic enforcement of perJobHistoryGoal tasks.

The first one drives the cleanup of finished tasks while the second one 
makes sure a flapping job does not produce too much junk. 

If we are to enforce min retention policy we need to do it for both. That 
is what happens now:
- calculateTimeout() produces the max retaining interval (up to 2 days)
- safeToDelete() checks if min retaining interval is satisfied before 
enforcing perJobHistoryGoal.

Given the above, I am not sure what I am to do with Math.max in that filter.


- Maxim Khutornenko


On March 14, 2014, 10:54 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19243/
> ---
> 
> (Updated March 14, 2014, 10:54 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-263
> https://issues.apache.org/jira/browse/AURORA-263
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding min retention interval for task history.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java 
> be67e7d1f93584542a645ada4499e26ab9bb1fb4 
>   src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java 
> 5bf9838e56f074fc5e66ace2a9c4e7bc50d1480f 
>   src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 
> f7c9e5ed408b44e93df8cfff9c9beeb2f717b2d2 
> 
> Diff: https://reviews.apache.org/r/19243/diff/
> 
> 
> Testing
> ---
> 
> gradle build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 19243: Adding min retention interval for task history.

[Bill Farner]

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



src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java


The decision will be purely arbitrary, but how about defaulting to an hour 
instead?  This gives someone a reasonable chance to notice a task in the system 
before it's expunged.



src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java


Can you make this settable via command line?



src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java


This filter suggests lack of confidence in calculateTimeout (duplicating 
behavior).  Consider either pushing the Math.max down here or dropping the 
filter.


- Bill Farner


On March 14, 2014, 10:54 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19243/
> ---
> 
> (Updated March 14, 2014, 10:54 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-263
> https://issues.apache.org/jira/browse/AURORA-263
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding min retention interval for task history.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java 
> be67e7d1f93584542a645ada4499e26ab9bb1fb4 
>   src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java 
> 5bf9838e56f074fc5e66ace2a9c4e7bc50d1480f 
>   src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 
> f7c9e5ed408b44e93df8cfff9c9beeb2f717b2d2 
> 
> Diff: https://reviews.apache.org/r/19243/diff/
> 
> 
> Testing
> ---
> 
> gradle build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 19243: Adding min retention interval for task history.

[Maxim Khutornenko]

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


Ping.

- Maxim Khutornenko


On March 14, 2014, 10:54 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19243/
> ---
> 
> (Updated March 14, 2014, 10:54 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-263
> https://issues.apache.org/jira/browse/AURORA-263
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding min retention interval for task history.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java 
> be67e7d1f93584542a645ada4499e26ab9bb1fb4 
>   src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java 
> 5bf9838e56f074fc5e66ace2a9c4e7bc50d1480f 
>   src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 
> f7c9e5ed408b44e93df8cfff9c9beeb2f717b2d2 
> 
> Diff: https://reviews.apache.org/r/19243/diff/
> 
> 
> Testing
> ---
> 
> gradle build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>