Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[Maxim Khutornenko]

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



src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java (lines 
152 - 161)


Am I reading this as a busy loop consuming 100% thread CPU waiting for 
something that may never happen? I don't think this is an acceptable solution.

Perhaps it's time to refactor task prunner into an 
AbstractScheduledService? I always felt task prunner approach of holding on to 
task IDs for 2 days just to act once on them isn't very efficient. What if 
instead of acting on every particular task ID we have a periodic (say every 30 
seconds) run loop to prune job keys instead?

Implementation-wise, it could be a Set of unique job keys that we populate 
on every TaskStateChange event. A runOneIteration() would poll that set and 
apply both latency and max_per_job thresholds for all related terminal tasks 
within the same iteration.

The only downside for the above is a somewhat increased history count 
between the cleanup runs but given that our current thresholds are chosen 
mostly arbitrarily I think that should be acceptable.


- Maxim Khutornenko


On Jan. 20, 2016, 10:39 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 20, 2016, 10:39 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is key to operating a large cluster and failure to prune should 
> trigger shutdown to prevent unbounded growth of storage. This patch turns 
> `TaskHistoryPruner` into a service which propagates failure from failed 
> pruning attempts towards the `ServiceManager`. Also completing a TODO which 
> removes a test for behaviour that is very awkward to test for.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
> 735199ac1ab343c24471890aa330d6635c26 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> e2e tests
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[John Sirois]

> On Jan. 21, 2016, 11:51 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java, 
> > lines 152-161
> > 
> >
> > Am I reading this as a busy loop consuming 100% thread CPU waiting for 
> > something that may never happen? I don't think this is an acceptable 
> > solution.
> > 
> > Perhaps it's time to refactor task prunner into an 
> > AbstractScheduledService? I always felt task prunner approach of holding on 
> > to task IDs for 2 days just to act once on them isn't very efficient. What 
> > if instead of acting on every particular task ID we have a periodic (say 
> > every 30 seconds) run loop to prune job keys instead?
> > 
> > Implementation-wise, it could be a Set of unique job keys that we 
> > populate on every TaskStateChange event. A runOneIteration() would poll 
> > that set and apply both latency and max_per_job thresholds for all related 
> > terminal tasks within the same iteration.
> > 
> > The only downside for the above is a somewhat increased history count 
> > between the cleanup runs but given that our current thresholds are chosen 
> > mostly arbitrarily I think that should be acceptable.

I think my Future/Queue suggestion above solves the busy loop with no liveness 
penalty.  That might allow your batching change suggestion to happen in a 
seperate follow-up RB.


- John


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


On Jan. 20, 2016, 3:39 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 20, 2016, 3:39 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is key to operating a large cluster and failure to prune should 
> trigger shutdown to prevent unbounded growth of storage. This patch turns 
> `TaskHistoryPruner` into a service which propagates failure from failed 
> pruning attempts towards the `ServiceManager`. Also completing a TODO which 
> removes a test for behaviour that is very awkward to test for.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
> 735199ac1ab343c24471890aa330d6635c26 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> e2e tests
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[John Sirois]

> On Jan. 20, 2016, 4:39 p.m., John Sirois wrote:
> > Ship It!

As Maxim points out below, the current `run` will consume a thread, so ship-it 
retracted!


- John


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


On Jan. 20, 2016, 3:39 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 20, 2016, 3:39 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is key to operating a large cluster and failure to prune should 
> trigger shutdown to prevent unbounded growth of storage. This patch turns 
> `TaskHistoryPruner` into a service which propagates failure from failed 
> pruning attempts towards the `ServiceManager`. Also completing a TODO which 
> removes a test for behaviour that is very awkward to test for.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
> 735199ac1ab343c24471890aa330d6635c26 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> e2e tests
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[Zameer Manji]

> On Jan. 21, 2016, 10:51 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java, 
> > lines 152-161
> > 
> >
> > Am I reading this as a busy loop consuming 100% thread CPU waiting for 
> > something that may never happen? I don't think this is an acceptable 
> > solution.
> > 
> > Perhaps it's time to refactor task prunner into an 
> > AbstractScheduledService? I always felt task prunner approach of holding on 
> > to task IDs for 2 days just to act once on them isn't very efficient. What 
> > if instead of acting on every particular task ID we have a periodic (say 
> > every 30 seconds) run loop to prune job keys instead?
> > 
> > Implementation-wise, it could be a Set of unique job keys that we 
> > populate on every TaskStateChange event. A runOneIteration() would poll 
> > that set and apply both latency and max_per_job thresholds for all related 
> > terminal tasks within the same iteration.
> > 
> > The only downside for the above is a somewhat increased history count 
> > between the cleanup runs but given that our current thresholds are chosen 
> > mostly arbitrarily I think that should be acceptable.
> 
> John Sirois wrote:
> I think my Future/Queue suggestion above solves the busy loop with no 
> liveness penalty.  That might allow your batching change suggestion to happen 
> in a seperate follow-up RB.

+1 to John here. I think we are overdue for a less complex and heavy pruner but 
I would prefer to keep this RB focused on failure propagation. I am open to a 
follow up ticket and RB. Maxim, if you agree, I can create a ticket that tracks 
the work you just proposed.

Right now, I think I will use the Future/Queue suggestion that John has to 
remove the busy loop.


- Zameer


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


On Jan. 20, 2016, 2:39 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 20, 2016, 2:39 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is key to operating a large cluster and failure to prune should 
> trigger shutdown to prevent unbounded growth of storage. This patch turns 
> `TaskHistoryPruner` into a service which propagates failure from failed 
> pruning attempts towards the `ServiceManager`. Also completing a TODO which 
> removes a test for behaviour that is very awkward to test for.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
> 735199ac1ab343c24471890aa330d6635c26 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> e2e tests
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Review Request 42613: Enable READ COMMITTED transaction isolation.

[Bill Farner]

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

Review request for Aurora, John Sirois and Zameer Manji.


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


Repository: aurora


Description
---

Proposing this as the minimal fix for AURORA-1580.  I think it's wise, however, 
to look towards the alternative approach i described in that ticket for 
simplicity and performance gains.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
2b3ee7bf6f7801c140f921b25f78daf6d320098a 
  src/main/resources/org/apache/aurora/scheduler/storage/db/TaskMapper.xml 
e689cfeda39e1f065ed29c61a23376512df8f3aa 
  src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
3dba286556ab6ea34b7fdd38508a164c902d9d17 

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


Testing
---


Thanks,

Bill Farner

Re: Review Request 42613: Enable READ COMMITTED transaction isolation.

[Bill Farner]

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

(Updated Jan. 21, 2016, 12:25 p.m.)


Review request for Aurora, John Sirois and Zameer Manji.


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


Repository: aurora


Description (updated)
---

Proposing this as the minimal fix for AURORA-1580.  I think it's wise, however, 
to look towards the alternative approach i described in that ticket for 
simplicity and performance gains.

I'd like to note that after this change, it will be important to treat 
long-running transactions carefully.  It will be important to avoid 
transactions block others by writing (locking a table) and subsequently doing 
other lengthy work.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
2b3ee7bf6f7801c140f921b25f78daf6d320098a 
  src/main/resources/org/apache/aurora/scheduler/storage/db/TaskMapper.xml 
e689cfeda39e1f065ed29c61a23376512df8f3aa 
  src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
3dba286556ab6ea34b7fdd38508a164c902d9d17 

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


Testing
---


Thanks,

Bill Farner

Re: Review Request 42613: Enable READ COMMITTED transaction isolation.

[John Sirois]

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

Ship it!


IIUC the MVCC capabilities in 1.4+ should prevent table locks on all but DDL 
stuff.


src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java (line 115)


Please fix the comment and add a pointer to 
http://www.h2database.com/html/advanced.html?highlight=LOCK_MODE&search=LOCK#transaction_isolation
 since "3" is so cryptic and folks - like me just now - will want to double 
check.


- John Sirois


On Jan. 21, 2016, 1:25 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42613/
> ---
> 
> (Updated Jan. 21, 2016, 1:25 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Bugs: AURORA-1580
> https://issues.apache.org/jira/browse/AURORA-1580
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Proposing this as the minimal fix for AURORA-1580.  I think it's wise, 
> however, to look towards the alternative approach i described in that ticket 
> for simplicity and performance gains.
> 
> I'd like to note that after this change, it will be important to treat 
> long-running transactions carefully.  It will be important to avoid 
> transactions block others by writing (locking a table) and subsequently doing 
> other lengthy work.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> 2b3ee7bf6f7801c140f921b25f78daf6d320098a 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/TaskMapper.xml 
> e689cfeda39e1f065ed29c61a23376512df8f3aa 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 3dba286556ab6ea34b7fdd38508a164c902d9d17 
> 
> Diff: https://reviews.apache.org/r/42613/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 42613: Enable READ COMMITTED transaction isolation.

[John Sirois]

> On Jan. 21, 2016, 1:28 p.m., John Sirois wrote:
> > IIUC the MVCC capabilities in 1.4+ should prevent table locks on all but 
> > DDL stuff.

Hmm - more to the story: "If MVCC is enabled, changing the lock mode 
(LOCK_MODE) has no effect." ... Maybe turning on MVCC is worth some perf tests 
or have you already been down that road?


- John


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


On Jan. 21, 2016, 1:25 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42613/
> ---
> 
> (Updated Jan. 21, 2016, 1:25 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Bugs: AURORA-1580
> https://issues.apache.org/jira/browse/AURORA-1580
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Proposing this as the minimal fix for AURORA-1580.  I think it's wise, 
> however, to look towards the alternative approach i described in that ticket 
> for simplicity and performance gains.
> 
> I'd like to note that after this change, it will be important to treat 
> long-running transactions carefully.  It will be important to avoid 
> transactions block others by writing (locking a table) and subsequently doing 
> other lengthy work.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> 2b3ee7bf6f7801c140f921b25f78daf6d320098a 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/TaskMapper.xml 
> e689cfeda39e1f065ed29c61a23376512df8f3aa 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 3dba286556ab6ea34b7fdd38508a164c902d9d17 
> 
> Diff: https://reviews.apache.org/r/42613/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 42613: Enable READ COMMITTED transaction isolation.

[Maxim Khutornenko]

> On Jan. 21, 2016, 8:28 p.m., John Sirois wrote:
> > IIUC the MVCC capabilities in 1.4+ should prevent table locks on all but 
> > DDL stuff.
> 
> John Sirois wrote:
> Hmm - more to the story: "If MVCC is enabled, changing the lock mode 
> (LOCK_MODE) has no effect." ... Maybe turning on MVCC is worth some perf 
> tests or have you already been down that road?

I suggest a cautios approach to enabling MVCC as our previous experience showed 
unacceptable performance with any meaningful amounts of data.


- Maxim


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


On Jan. 21, 2016, 8:25 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42613/
> ---
> 
> (Updated Jan. 21, 2016, 8:25 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Bugs: AURORA-1580
> https://issues.apache.org/jira/browse/AURORA-1580
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Proposing this as the minimal fix for AURORA-1580.  I think it's wise, 
> however, to look towards the alternative approach i described in that ticket 
> for simplicity and performance gains.
> 
> I'd like to note that after this change, it will be important to treat 
> long-running transactions carefully.  It will be important to avoid 
> transactions block others by writing (locking a table) and subsequently doing 
> other lengthy work.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> 2b3ee7bf6f7801c140f921b25f78daf6d320098a 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/TaskMapper.xml 
> e689cfeda39e1f065ed29c61a23376512df8f3aa 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 3dba286556ab6ea34b7fdd38508a164c902d9d17 
> 
> Diff: https://reviews.apache.org/r/42613/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 42613: Enable READ COMMITTED transaction isolation.

[Maxim Khutornenko]

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

Ship it!


Ship It!

- Maxim Khutornenko


On Jan. 21, 2016, 8:25 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42613/
> ---
> 
> (Updated Jan. 21, 2016, 8:25 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Bugs: AURORA-1580
> https://issues.apache.org/jira/browse/AURORA-1580
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Proposing this as the minimal fix for AURORA-1580.  I think it's wise, 
> however, to look towards the alternative approach i described in that ticket 
> for simplicity and performance gains.
> 
> I'd like to note that after this change, it will be important to treat 
> long-running transactions carefully.  It will be important to avoid 
> transactions block others by writing (locking a table) and subsequently doing 
> other lengthy work.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> 2b3ee7bf6f7801c140f921b25f78daf6d320098a 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/TaskMapper.xml 
> e689cfeda39e1f065ed29c61a23376512df8f3aa 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 3dba286556ab6ea34b7fdd38508a164c902d9d17 
> 
> Diff: https://reviews.apache.org/r/42613/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 42613: Enable READ COMMITTED transaction isolation.

[Bill Farner]

> On Jan. 21, 2016, 12:28 p.m., John Sirois wrote:
> > IIUC the MVCC capabilities in 1.4+ should prevent table locks on all but 
> > DDL stuff.
> 
> John Sirois wrote:
> Hmm - more to the story: "If MVCC is enabled, changing the lock mode 
> (LOCK_MODE) has no effect." ... Maybe turning on MVCC is worth some perf 
> tests or have you already been down that road?
> 
> Maxim Khutornenko wrote:
> I suggest a cautios approach to enabling MVCC as our previous experience 
> showed unacceptable performance with any meaningful amounts of data.

Bigger caution for me is that MVCC is considered beta in H2.


> On Jan. 21, 2016, 12:28 p.m., John Sirois wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 115
> > 
> >
> > Please fix the comment and add a pointer to 
> > http://www.h2database.com/html/advanced.html?highlight=LOCK_MODE&search=LOCK#transaction_isolation
> >  since "3" is so cryptic and folks - like me just now - will want to double 
> > check.

Agreed, fixed.


- Bill


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


On Jan. 21, 2016, 12:25 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42613/
> ---
> 
> (Updated Jan. 21, 2016, 12:25 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Bugs: AURORA-1580
> https://issues.apache.org/jira/browse/AURORA-1580
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Proposing this as the minimal fix for AURORA-1580.  I think it's wise, 
> however, to look towards the alternative approach i described in that ticket 
> for simplicity and performance gains.
> 
> I'd like to note that after this change, it will be important to treat 
> long-running transactions carefully.  It will be important to avoid 
> transactions block others by writing (locking a table) and subsequently doing 
> other lengthy work.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> 2b3ee7bf6f7801c140f921b25f78daf6d320098a 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/TaskMapper.xml 
> e689cfeda39e1f065ed29c61a23376512df8f3aa 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 3dba286556ab6ea34b7fdd38508a164c902d9d17 
> 
> Diff: https://reviews.apache.org/r/42613/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 42613: Enable READ COMMITTED transaction isolation.

[Bill Farner]

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

(Updated Jan. 21, 2016, 12:40 p.m.)


Review request for Aurora, John Sirois and Zameer Manji.


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


Repository: aurora


Description
---

Proposing this as the minimal fix for AURORA-1580.  I think it's wise, however, 
to look towards the alternative approach i described in that ticket for 
simplicity and performance gains.

I'd like to note that after this change, it will be important to treat 
long-running transactions carefully.  It will be important to avoid 
transactions block others by writing (locking a table) and subsequently doing 
other lengthy work.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
2b3ee7bf6f7801c140f921b25f78daf6d320098a 
  src/main/resources/org/apache/aurora/scheduler/storage/db/TaskMapper.xml 
e689cfeda39e1f065ed29c61a23376512df8f3aa 
  src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
3dba286556ab6ea34b7fdd38508a164c902d9d17 

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


Testing
---


Thanks,

Bill Farner

Re: Review Request 42613: Enable READ COMMITTED transaction isolation.

[John Sirois]

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

Ship it!


Ship It!

- John Sirois


On Jan. 21, 2016, 1:40 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42613/
> ---
> 
> (Updated Jan. 21, 2016, 1:40 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Bugs: AURORA-1580
> https://issues.apache.org/jira/browse/AURORA-1580
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Proposing this as the minimal fix for AURORA-1580.  I think it's wise, 
> however, to look towards the alternative approach i described in that ticket 
> for simplicity and performance gains.
> 
> I'd like to note that after this change, it will be important to treat 
> long-running transactions carefully.  It will be important to avoid 
> transactions block others by writing (locking a table) and subsequently doing 
> other lengthy work.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> 2b3ee7bf6f7801c140f921b25f78daf6d320098a 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/TaskMapper.xml 
> e689cfeda39e1f065ed29c61a23376512df8f3aa 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 3dba286556ab6ea34b7fdd38508a164c902d9d17 
> 
> Diff: https://reviews.apache.org/r/42613/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 42613: Enable READ COMMITTED transaction isolation.

[John Sirois]

> On Jan. 21, 2016, 1:28 p.m., John Sirois wrote:
> > IIUC the MVCC capabilities in 1.4+ should prevent table locks on all but 
> > DDL stuff.
> 
> John Sirois wrote:
> Hmm - more to the story: "If MVCC is enabled, changing the lock mode 
> (LOCK_MODE) has no effect." ... Maybe turning on MVCC is worth some perf 
> tests or have you already been down that road?
> 
> Maxim Khutornenko wrote:
> I suggest a cautios approach to enabling MVCC as our previous experience 
> showed unacceptable performance with any meaningful amounts of data.
> 
> Bill Farner wrote:
> Bigger caution for me is that MVCC is considered beta in H2.

The beta bit is not at all clear to me.  If I read like a lawyer, I _think_ I 
agree, but I don;t find this all that clear!:
> The MVCC mode is enabled by default in version 1.4.x, with the default 
> MVStore storage engine. MVCC is disabled by default when using the PageStore 
> storage engine (which is the default in version 1.3.x). The following applies 
> when using the PageStore storage engine: The MVCC feature is not fully tested 
> yet.


- John


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


On Jan. 21, 2016, 1:40 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42613/
> ---
> 
> (Updated Jan. 21, 2016, 1:40 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Bugs: AURORA-1580
> https://issues.apache.org/jira/browse/AURORA-1580
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Proposing this as the minimal fix for AURORA-1580.  I think it's wise, 
> however, to look towards the alternative approach i described in that ticket 
> for simplicity and performance gains.
> 
> I'd like to note that after this change, it will be important to treat 
> long-running transactions carefully.  It will be important to avoid 
> transactions block others by writing (locking a table) and subsequently doing 
> other lengthy work.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> 2b3ee7bf6f7801c140f921b25f78daf6d320098a 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/TaskMapper.xml 
> e689cfeda39e1f065ed29c61a23376512df8f3aa 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 3dba286556ab6ea34b7fdd38508a164c902d9d17 
> 
> Diff: https://reviews.apache.org/r/42613/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Review Request 42614: Allowing dual authorizing params to account for thrift API deprecations.

[Maxim Khutornenko]

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

Review request for Aurora and Bill Farner.


Repository: aurora


Description
---

We don't allow dual annotations for authorizing params in thrift. There are 
cases, however, when it's needed. One example is AURORA-1583 where we would 
need to support something like below before we cutoff `query` in the next 
release:
```
  Response killTasks(
  @AuthorizingParam @Nullable TaskQuery query,
  @Nullable Lock lock,
  @AuthorizingParam @Nullable JobKey job,
  int instances) throws TException;
```  

The new behavior allows at most 2 annotations but only one of the annotated 
arguments must be non-null during interception.

Also, added missing test coverage.


Diffs
-

  config/legacy_untested_classes.txt 6b71fd233af0d137332bc69249d16e433aa198c7 
  
src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
 11d7e465556020571ffeefcf05596e6251ba9d19 
  
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
 69056624064be4bbd4136afb4dd6e3eb33c5e0d2 
  
src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java
 16a3a3b84c2a06bc340575abb58f853a8f26920d 

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


Testing
---


Thanks,

Maxim Khutornenko

Re: Review Request 42613: Enable READ COMMITTED transaction isolation.

[Aurora ReviewBot]

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

Ship it!


Master (8d3fb24) 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. 21, 2016, 8:40 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42613/
> ---
> 
> (Updated Jan. 21, 2016, 8:40 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Bugs: AURORA-1580
> https://issues.apache.org/jira/browse/AURORA-1580
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Proposing this as the minimal fix for AURORA-1580.  I think it's wise, 
> however, to look towards the alternative approach i described in that ticket 
> for simplicity and performance gains.
> 
> I'd like to note that after this change, it will be important to treat 
> long-running transactions carefully.  It will be important to avoid 
> transactions block others by writing (locking a table) and subsequently doing 
> other lengthy work.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> 2b3ee7bf6f7801c140f921b25f78daf6d320098a 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/TaskMapper.xml 
> e689cfeda39e1f065ed29c61a23376512df8f3aa 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 3dba286556ab6ea34b7fdd38508a164c902d9d17 
> 
> Diff: https://reviews.apache.org/r/42613/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 42614: Allowing dual authorizing params to account for thrift API deprecations.

[Bill Farner]

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



src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
 (line 34)


The limit of 2 seems arbitrary, and this reflects in the code.  Why not 
simply say that if there are multiple authorizing params, exactly one must be 
non-null?



src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
 (line 209)


Consider removing the use of guava's `Invokable` and `Parameter` 
throughout.  They really only exist as an abstraction over methods and 
constructors, which we don't need here.  The change is self-explanatory 
starting with

```
candidateMethod.getParameters()
```



src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
 (line 215)


Not new, but consider dropping this check.  The lookup in 
`FIELD_GETTERS_BY_TYPE` is more direct.



src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
 (line 220)


s/final //


- Bill Farner


On Jan. 21, 2016, 12:53 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42614/
> ---
> 
> (Updated Jan. 21, 2016, 12:53 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> We don't allow dual annotations for authorizing params in thrift. There are 
> cases, however, when it's needed. One example is AURORA-1583 where we would 
> need to support something like below before we cutoff `query` in the next 
> release:
> ```
>   Response killTasks(
>   @AuthorizingParam @Nullable TaskQuery query,
>   @Nullable Lock lock,
>   @AuthorizingParam @Nullable JobKey job,
>   int instances) throws TException;
> ```  
> 
> The new behavior allows at most 2 annotations but only one of the annotated 
> arguments must be non-null during interception.
> 
> Also, added missing test coverage.
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt 6b71fd233af0d137332bc69249d16e433aa198c7 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
>  11d7e465556020571ffeefcf05596e6251ba9d19 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
>  69056624064be4bbd4136afb4dd6e3eb33c5e0d2 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java
>  16a3a3b84c2a06bc340575abb58f853a8f26920d 
> 
> Diff: https://reviews.apache.org/r/42614/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 42613: Enable READ COMMITTED transaction isolation.

[Zameer Manji]

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

Ship it!


Ship It!

- Zameer Manji


On Jan. 21, 2016, 12:40 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42613/
> ---
> 
> (Updated Jan. 21, 2016, 12:40 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Bugs: AURORA-1580
> https://issues.apache.org/jira/browse/AURORA-1580
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Proposing this as the minimal fix for AURORA-1580.  I think it's wise, 
> however, to look towards the alternative approach i described in that ticket 
> for simplicity and performance gains.
> 
> I'd like to note that after this change, it will be important to treat 
> long-running transactions carefully.  It will be important to avoid 
> transactions block others by writing (locking a table) and subsequently doing 
> other lengthy work.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> 2b3ee7bf6f7801c140f921b25f78daf6d320098a 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/TaskMapper.xml 
> e689cfeda39e1f065ed29c61a23376512df8f3aa 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 3dba286556ab6ea34b7fdd38508a164c902d9d17 
> 
> Diff: https://reviews.apache.org/r/42613/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 42614: Allowing dual authorizing params to account for thrift API deprecations.

[Maxim Khutornenko]

> On Jan. 21, 2016, 9:14 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java,
> >  line 34
> > 
> >
> > The limit of 2 seems arbitrary, and this reflects in the code.  Why not 
> > simply say that if there are multiple authorizing params, exactly one must 
> > be non-null?

This was a deliberate choice to address thrift param deprecation case. We 
currently allow only one annotation and given our backwards v-1 compatibilty 
story I don't really see a need for more than 2 annotations to account for 
migration. That said, I am fine opening it up and allow multiples as what 
really counts is the final argument check.


> On Jan. 21, 2016, 9:14 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java,
> >  line 210
> > 
> >
> > Consider removing the use of guava's `Invokable` and `Parameter` 
> > throughout.  They really only exist as an abstraction over methods and 
> > constructors, which we don't need here.  The change is self-explanatory 
> > starting with
> > 
> > ```
> > candidateMethod.getParameters()
> > ```

Done.


> On Jan. 21, 2016, 9:14 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java,
> >  line 216
> > 
> >
> > Not new, but consider dropping this check.  The lookup in 
> > `FIELD_GETTERS_BY_TYPE` is more direct.

Dropped.


> On Jan. 21, 2016, 9:14 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java,
> >  line 221
> > 
> >
> > s/final //

Done.


- Maxim


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


On Jan. 21, 2016, 8:53 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42614/
> ---
> 
> (Updated Jan. 21, 2016, 8:53 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> We don't allow dual annotations for authorizing params in thrift. There are 
> cases, however, when it's needed. One example is AURORA-1583 where we would 
> need to support something like below before we cutoff `query` in the next 
> release:
> ```
>   Response killTasks(
>   @AuthorizingParam @Nullable TaskQuery query,
>   @Nullable Lock lock,
>   @AuthorizingParam @Nullable JobKey job,
>   int instances) throws TException;
> ```  
> 
> The new behavior allows at most 2 annotations but only one of the annotated 
> arguments must be non-null during interception.
> 
> Also, added missing test coverage.
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt 6b71fd233af0d137332bc69249d16e433aa198c7 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
>  11d7e465556020571ffeefcf05596e6251ba9d19 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
>  69056624064be4bbd4136afb4dd6e3eb33c5e0d2 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java
>  16a3a3b84c2a06bc340575abb58f853a8f26920d 
> 
> Diff: https://reviews.apache.org/r/42614/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 42614: Allowing dual authorizing params to account for thrift API deprecations.

[Maxim Khutornenko]

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

(Updated Jan. 21, 2016, 10:05 p.m.)


Review request for Aurora and Bill Farner.


Changes
---

Bill's comments.


Repository: aurora


Description
---

We don't allow dual annotations for authorizing params in thrift. There are 
cases, however, when it's needed. One example is AURORA-1583 where we would 
need to support something like below before we cutoff `query` in the next 
release:
```
  Response killTasks(
  @AuthorizingParam @Nullable TaskQuery query,
  @Nullable Lock lock,
  @AuthorizingParam @Nullable JobKey job,
  int instances) throws TException;
```  

The new behavior allows at most 2 annotations but only one of the annotated 
arguments must be non-null during interception.

Also, added missing test coverage.


Diffs (updated)
-

  config/legacy_untested_classes.txt 6b71fd233af0d137332bc69249d16e433aa198c7 
  
src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
 11d7e465556020571ffeefcf05596e6251ba9d19 
  
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
 69056624064be4bbd4136afb4dd6e3eb33c5e0d2 
  
src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java
 16a3a3b84c2a06bc340575abb58f853a8f26920d 

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


Testing
---


Thanks,

Maxim Khutornenko

Re: Review Request 42614: Allowing dual authorizing params to account for thrift API deprecations.

[Bill Farner]

> On Jan. 21, 2016, 1:14 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java,
> >  line 34
> > 
> >
> > The limit of 2 seems arbitrary, and this reflects in the code.  Why not 
> > simply say that if there are multiple authorizing params, exactly one must 
> > be non-null?
> 
> Maxim Khutornenko wrote:
> This was a deliberate choice to address thrift param deprecation case. We 
> currently allow only one annotation and given our backwards v-1 compatibilty 
> story I don't really see a need for more than 2 annotations to account for 
> migration. That said, I am fine opening it up and allow multiples as what 
> really counts is the final argument check.

Thanks, this is a situation where there's little reason to inhibit future 
unforeseen use cases.


- Bill


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


On Jan. 21, 2016, 2:05 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42614/
> ---
> 
> (Updated Jan. 21, 2016, 2:05 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> We don't allow dual annotations for authorizing params in thrift. There are 
> cases, however, when it's needed. One example is AURORA-1583 where we would 
> need to support something like below before we cutoff `query` in the next 
> release:
> ```
>   Response killTasks(
>   @AuthorizingParam @Nullable TaskQuery query,
>   @Nullable Lock lock,
>   @AuthorizingParam @Nullable JobKey job,
>   int instances) throws TException;
> ```  
> 
> The new behavior allows at most 2 annotations but only one of the annotated 
> arguments must be non-null during interception.
> 
> Also, added missing test coverage.
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt 6b71fd233af0d137332bc69249d16e433aa198c7 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
>  11d7e465556020571ffeefcf05596e6251ba9d19 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
>  69056624064be4bbd4136afb4dd6e3eb33c5e0d2 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java
>  16a3a3b84c2a06bc340575abb58f853a8f26920d 
> 
> Diff: https://reviews.apache.org/r/42614/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 42614: Allowing dual authorizing params to account for thrift API deprecations.

[Bill Farner]

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

Ship it!


Before submitting, please verify that end-to-end tests work with this patch.

- Bill Farner


On Jan. 21, 2016, 2:05 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42614/
> ---
> 
> (Updated Jan. 21, 2016, 2:05 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> We don't allow dual annotations for authorizing params in thrift. There are 
> cases, however, when it's needed. One example is AURORA-1583 where we would 
> need to support something like below before we cutoff `query` in the next 
> release:
> ```
>   Response killTasks(
>   @AuthorizingParam @Nullable TaskQuery query,
>   @Nullable Lock lock,
>   @AuthorizingParam @Nullable JobKey job,
>   int instances) throws TException;
> ```  
> 
> The new behavior allows at most 2 annotations but only one of the annotated 
> arguments must be non-null during interception.
> 
> Also, added missing test coverage.
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt 6b71fd233af0d137332bc69249d16e433aa198c7 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
>  11d7e465556020571ffeefcf05596e6251ba9d19 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
>  69056624064be4bbd4136afb4dd6e3eb33c5e0d2 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java
>  16a3a3b84c2a06bc340575abb58f853a8f26920d 
> 
> Diff: https://reviews.apache.org/r/42614/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[Maxim Khutornenko]

> On Jan. 21, 2016, 6:51 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java, 
> > lines 152-161
> > 
> >
> > Am I reading this as a busy loop consuming 100% thread CPU waiting for 
> > something that may never happen? I don't think this is an acceptable 
> > solution.
> > 
> > Perhaps it's time to refactor task prunner into an 
> > AbstractScheduledService? I always felt task prunner approach of holding on 
> > to task IDs for 2 days just to act once on them isn't very efficient. What 
> > if instead of acting on every particular task ID we have a periodic (say 
> > every 30 seconds) run loop to prune job keys instead?
> > 
> > Implementation-wise, it could be a Set of unique job keys that we 
> > populate on every TaskStateChange event. A runOneIteration() would poll 
> > that set and apply both latency and max_per_job thresholds for all related 
> > terminal tasks within the same iteration.
> > 
> > The only downside for the above is a somewhat increased history count 
> > between the cleanup runs but given that our current thresholds are chosen 
> > mostly arbitrarily I think that should be acceptable.
> 
> John Sirois wrote:
> I think my Future/Queue suggestion above solves the busy loop with no 
> liveness penalty.  That might allow your batching change suggestion to happen 
> in a seperate follow-up RB.
> 
> Zameer Manji wrote:
> +1 to John here. I think we are overdue for a less complex and heavy 
> pruner but I would prefer to keep this RB focused on failure propagation. I 
> am open to a follow up ticket and RB. Maxim, if you agree, I can create a 
> ticket that tracks the work you just proposed.
> 
> Right now, I think I will use the Future/Queue suggestion that John has 
> to remove the busy loop.

I am fine with the follow up ticket if you feel it's too much to lift within 
this RB.


- Maxim


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


On Jan. 20, 2016, 10:39 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 20, 2016, 10:39 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is key to operating a large cluster and failure to prune should 
> trigger shutdown to prevent unbounded growth of storage. This patch turns 
> `TaskHistoryPruner` into a service which propagates failure from failed 
> pruning attempts towards the `ServiceManager`. Also completing a TODO which 
> removes a test for behaviour that is very awkward to test for.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
> 735199ac1ab343c24471890aa330d6635c26 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> e2e tests
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42387: working version of jessie builds

[Dmitriy Shirchenko]

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

(Updated Jan. 21, 2016, 10:20 p.m.)


Review request for Aurora, Benjamin Staffin, John Sirois, and Bill Farner.


Repository: aurora-packaging


Description
---

Adding debian jessie packaging with a test.


Diffs (updated)
-

  builder/deb/debian-jessie/Dockerfile PRE-CREATION 
  builder/deb/debian-jessie/build.sh PRE-CREATION 
  builder/deb/debian-jessie/pants.ini PRE-CREATION 
  specs/debian/aurora-executor.thermos.init 
e5c940147f5f0a1842d15be9bb9c44e838bc9af0 
  specs/debian/aurora-executor.thermos.service PRE-CREATION 
  specs/debian/aurora-scheduler.init 59b200fcd9029e14ea63fe8edb159022e7bde346 
  specs/debian/aurora-scheduler.service PRE-CREATION 
  specs/debian/aurora-scheduler.upstart 
1ceb7042485efedcf7136b51737107aada4c4e96 
  specs/rpm/SOURCES/aurora.sysconfig f421d19db007960764054ea10b5fc3d3478f2a15 
  test/deb/debian-jessie/README.md PRE-CREATION 
  test/deb/debian-jessie/Vagrantfile PRE-CREATION 
  test/deb/debian-jessie/provision.sh PRE-CREATION 

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


Testing
---

New build with a test that starts a Vagrant and following README should lead to 
a successful job.


Thanks,

Dmitriy Shirchenko

Re: Review Request 42387: working version of jessie builds

[Dmitriy Shirchenko]

> On Jan. 21, 2016, 1:26 a.m., Bill Farner wrote:
> > build-artifact.sh, line 35
> > 
> >
> > How about a selective `set -x`, `set +x` around the commands you wish 
> > to see echoed instead?

Removed. Adding set -x, set +x to my arsenal when debugging shell scripts. 
Thanks.


> On Jan. 21, 2016, 1:26 a.m., Bill Farner wrote:
> > builder/deb/debian-jessie/Dockerfile, line 37
> > 
> >
> > FYI - the deb has a build-time dep of a package named 'gradle', so it 
> > would be slightly more involved than that.

Thanks. Clarified comment.


> On Jan. 21, 2016, 1:26 a.m., Bill Farner wrote:
> > builder/deb/debian-jessie/build.sh, line 1
> > 
> >
> > Checking my recollection - this can't be a symlink due to the way it's 
> > mounted with docker, right?  If that's correct, can you leave a TODO for me 
> > to figure out how to share these files?

Sure. There could be an easy way to mounting the file into the container, but 
yea, leaving a TODO.


> On Jan. 21, 2016, 1:26 a.m., Bill Farner wrote:
> > specs/debian/aurora-executor.thermos.init, lines 43-44
> > 
> >
> > Is this necessary due to `app_daemonize` or is there more to it?

im not really sure tbh. these were suggestions from +benley.

but are you asking about the format change or log_dir change? log_to_disk was 
supposed to make it consistent and it was told me to it's better. 

i am not sure what log_dir was before, but now it's explicit and i see logs 
going there.


- Dmitriy


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


On Jan. 20, 2016, 12:59 a.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42387/
> ---
> 
> (Updated Jan. 20, 2016, 12:59 a.m.)
> 
> 
> Review request for Aurora, Benjamin Staffin, John Sirois, and Bill Farner.
> 
> 
> Repository: aurora-packaging
> 
> 
> Description
> ---
> 
> Adding debian jessie packaging with a test.
> 
> 
> Diffs
> -
> 
>   build-artifact.sh 333c7a4d8e661f608c4949dcbae1401bb1a75b51 
>   builder/deb/debian-jessie/Dockerfile PRE-CREATION 
>   builder/deb/debian-jessie/build.sh PRE-CREATION 
>   builder/deb/debian-jessie/pants.ini PRE-CREATION 
>   specs/debian/aurora-executor.thermos.init 
> e5c940147f5f0a1842d15be9bb9c44e838bc9af0 
>   specs/debian/aurora-executor.thermos.service PRE-CREATION 
>   specs/debian/aurora-scheduler.init 59b200fcd9029e14ea63fe8edb159022e7bde346 
>   specs/debian/aurora-scheduler.service PRE-CREATION 
>   specs/debian/aurora-scheduler.upstart 
> 1ceb7042485efedcf7136b51737107aada4c4e96 
>   specs/rpm/SOURCES/aurora.sysconfig f421d19db007960764054ea10b5fc3d3478f2a15 
>   test/deb/debian-jessie/README.md PRE-CREATION 
>   test/deb/debian-jessie/Vagrantfile PRE-CREATION 
>   test/deb/debian-jessie/provision.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42387/diff/
> 
> 
> Testing
> ---
> 
> New build with a test that starts a Vagrant and following README should lead 
> to a successful job.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[Zameer Manji]

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

(Updated Jan. 21, 2016, 2:22 p.m.)


Review request for Aurora, John Sirois and Maxim Khutornenko.


Changes
---

Removed busy waiting for periodic checking.


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


Repository: aurora


Description
---

Task pruning is key to operating a large cluster and failure to prune should 
trigger shutdown to prevent unbounded growth of storage. This patch turns 
`TaskHistoryPruner` into a service which propagates failure from failed pruning 
attempts towards the `ServiceManager`. Also completing a TODO which removes a 
test for behaviour that is very awkward to test for.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
735199ac1ab343c24471890aa330d6635c26 
  src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
2064089937f5178b1413d386a312f4173a0e35fb 
  src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
295960f13693c6ba0d7075a8ef7f9680a91ae69d 

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


Testing
---

./gradlew build -Pq
e2e tests


Thanks,

Zameer Manji

Re: Review Request 42387: working version of jessie builds

[Bill Farner]

> On Jan. 20, 2016, 5:26 p.m., Bill Farner wrote:
> > specs/debian/aurora-executor.thermos.init, lines 43-44
> > 
> >
> > Is this necessary due to `app_daemonize` or is there more to it?
> 
> Dmitriy Shirchenko wrote:
> im not really sure tbh. these were suggestions from +benley.
> 
> but are you asking about the format change or log_dir change? log_to_disk 
> was supposed to make it consistent and it was told me to it's better. 
> 
> i am not sure what log_dir was before, but now it's explicit and i see 
> logs going there.

My only complaint is that the logging behavior will be inconsistent for 
operators.  At any rate, i'd rather ship and iterate.


- Bill


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


On Jan. 21, 2016, 2:20 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42387/
> ---
> 
> (Updated Jan. 21, 2016, 2:20 p.m.)
> 
> 
> Review request for Aurora, Benjamin Staffin, John Sirois, and Bill Farner.
> 
> 
> Repository: aurora-packaging
> 
> 
> Description
> ---
> 
> Adding debian jessie packaging with a test.
> 
> 
> Diffs
> -
> 
>   builder/deb/debian-jessie/Dockerfile PRE-CREATION 
>   builder/deb/debian-jessie/build.sh PRE-CREATION 
>   builder/deb/debian-jessie/pants.ini PRE-CREATION 
>   specs/debian/aurora-executor.thermos.init 
> e5c940147f5f0a1842d15be9bb9c44e838bc9af0 
>   specs/debian/aurora-executor.thermos.service PRE-CREATION 
>   specs/debian/aurora-scheduler.init 59b200fcd9029e14ea63fe8edb159022e7bde346 
>   specs/debian/aurora-scheduler.service PRE-CREATION 
>   specs/debian/aurora-scheduler.upstart 
> 1ceb7042485efedcf7136b51737107aada4c4e96 
>   specs/rpm/SOURCES/aurora.sysconfig f421d19db007960764054ea10b5fc3d3478f2a15 
>   test/deb/debian-jessie/README.md PRE-CREATION 
>   test/deb/debian-jessie/Vagrantfile PRE-CREATION 
>   test/deb/debian-jessie/provision.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42387/diff/
> 
> 
> Testing
> ---
> 
> New build with a test that starts a Vagrant and following README should lead 
> to a successful job.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 42387: working version of jessie builds

[Bill Farner]

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

Ship it!


Ship It!

- Bill Farner


On Jan. 21, 2016, 2:20 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42387/
> ---
> 
> (Updated Jan. 21, 2016, 2:20 p.m.)
> 
> 
> Review request for Aurora, Benjamin Staffin, John Sirois, and Bill Farner.
> 
> 
> Repository: aurora-packaging
> 
> 
> Description
> ---
> 
> Adding debian jessie packaging with a test.
> 
> 
> Diffs
> -
> 
>   builder/deb/debian-jessie/Dockerfile PRE-CREATION 
>   builder/deb/debian-jessie/build.sh PRE-CREATION 
>   builder/deb/debian-jessie/pants.ini PRE-CREATION 
>   specs/debian/aurora-executor.thermos.init 
> e5c940147f5f0a1842d15be9bb9c44e838bc9af0 
>   specs/debian/aurora-executor.thermos.service PRE-CREATION 
>   specs/debian/aurora-scheduler.init 59b200fcd9029e14ea63fe8edb159022e7bde346 
>   specs/debian/aurora-scheduler.service PRE-CREATION 
>   specs/debian/aurora-scheduler.upstart 
> 1ceb7042485efedcf7136b51737107aada4c4e96 
>   specs/rpm/SOURCES/aurora.sysconfig f421d19db007960764054ea10b5fc3d3478f2a15 
>   test/deb/debian-jessie/README.md PRE-CREATION 
>   test/deb/debian-jessie/Vagrantfile PRE-CREATION 
>   test/deb/debian-jessie/provision.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42387/diff/
> 
> 
> Testing
> ---
> 
> New build with a test that starts a Vagrant and following README should lead 
> to a successful job.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[Aurora ReviewBot]

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

Ship it!


Master (8d3fb24) 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. 21, 2016, 10:22 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 21, 2016, 10:22 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is key to operating a large cluster and failure to prune should 
> trigger shutdown to prevent unbounded growth of storage. This patch turns 
> `TaskHistoryPruner` into a service which propagates failure from failed 
> pruning attempts towards the `ServiceManager`. Also completing a TODO which 
> removes a test for behaviour that is very awkward to test for.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
> 735199ac1ab343c24471890aa330d6635c26 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> e2e tests
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42387: working version of jessie builds

[Benjamin Staffin]

> On Jan. 19, 2016, 4:12 p.m., John Sirois wrote:
> > builder/deb/debian-jessie/Dockerfile, line 38
> > 
> >
> > Its not exactly clear to me how this is any better than downloading 
> > gradle from gradle.  Since this is only used for the builder it seems to me 
> > the only relevant details is - not a standard package install - aka a 
> > ~random internet download.  If the plan is that benley's work here makes it 
> > upstream to a standard debian deb repo I think its worth a comment pointing 
> > that expected future out.  As it stands w/o that comment and forgetting we 
> > know and trust benley, this looks on the face decidedly worse than going 
> > straight to the gradle source.
> 
> Dmitriy Shirchenko wrote:
> Done.

I originally put together that gradle-packaging stuff to make it possible to 
build aurora inside of cowbuilder/pbuilder, where (at least in theory) all the 
build dependencies are supposed to be satisfiable with apt-get.  It's not as 
relevant to the docker-based process being used here, but removing it does mean 
there will be a build dep that isn't mentioned in the control file at all...


> On Jan. 19, 2016, 4:12 p.m., John Sirois wrote:
> > specs/debian/aurora-executor.thermos.init, line 22
> > 
> >
> > This is admittedly all over the place across our packages and OSs 
> > already before your change, but for the deb scheduler we have 
> > `/var/run/aurora-scheduler.pid` - how about `/var/run/aurora-observer.pid` 
> > or `/var/run/thermos-observer.pid`, some consistency here would be useful I 
> > think for discovery purposes.
> 
> Dmitriy Shirchenko wrote:
> Done.

I haven't seen this called aurora-observer anywhere else before.  I would have 
strongly preferred /var/run/thermos-observer.pid here, unless thermos observer 
itself is being renamed.


- Benjamin


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


On Jan. 21, 2016, 2:20 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42387/
> ---
> 
> (Updated Jan. 21, 2016, 2:20 p.m.)
> 
> 
> Review request for Aurora, Benjamin Staffin, John Sirois, and Bill Farner.
> 
> 
> Repository: aurora-packaging
> 
> 
> Description
> ---
> 
> Adding debian jessie packaging with a test.
> 
> 
> Diffs
> -
> 
>   builder/deb/debian-jessie/Dockerfile PRE-CREATION 
>   builder/deb/debian-jessie/build.sh PRE-CREATION 
>   builder/deb/debian-jessie/pants.ini PRE-CREATION 
>   specs/debian/aurora-executor.thermos.init 
> e5c940147f5f0a1842d15be9bb9c44e838bc9af0 
>   specs/debian/aurora-executor.thermos.service PRE-CREATION 
>   specs/debian/aurora-scheduler.init 59b200fcd9029e14ea63fe8edb159022e7bde346 
>   specs/debian/aurora-scheduler.service PRE-CREATION 
>   specs/debian/aurora-scheduler.upstart 
> 1ceb7042485efedcf7136b51737107aada4c4e96 
>   specs/rpm/SOURCES/aurora.sysconfig f421d19db007960764054ea10b5fc3d3478f2a15 
>   test/deb/debian-jessie/README.md PRE-CREATION 
>   test/deb/debian-jessie/Vagrantfile PRE-CREATION 
>   test/deb/debian-jessie/provision.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42387/diff/
> 
> 
> Testing
> ---
> 
> New build with a test that starts a Vagrant and following README should lead 
> to a successful job.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[Maxim Khutornenko]

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



src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java (line 
108)


Why BlockingQueue and not something like ConcurrentLinkedQueue? There is 
nothing to block here for.



src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java (line 
140)


runOneIteration() implies periodicity already, consider simplifying this 
comment



src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java (line 
153)


Does it have to run every second? This just checks for errors so a higher 
limit should suffice (e.g. 5 seconds).


- Maxim Khutornenko


On Jan. 21, 2016, 10:22 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 21, 2016, 10:22 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is key to operating a large cluster and failure to prune should 
> trigger shutdown to prevent unbounded growth of storage. This patch turns 
> `TaskHistoryPruner` into a service which propagates failure from failed 
> pruning attempts towards the `ServiceManager`. Also completing a TODO which 
> removes a test for behaviour that is very awkward to test for.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
> 735199ac1ab343c24471890aa330d6635c26 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> e2e tests
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[Zameer Manji]

> On Jan. 21, 2016, 2:50 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java, 
> > line 108
> > 
> >
> > Why BlockingQueue and not something like ConcurrentLinkedQueue? There 
> > is nothing to block here for.

`BlockingQueue` has the `drainTo` method which I think makes the iteration 
nicer.


> On Jan. 21, 2016, 2:50 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java, 
> > line 140
> > 
> >
> > runOneIteration() implies periodicity already, consider simplifying 
> > this comment

Simplified.


> On Jan. 21, 2016, 2:50 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java, 
> > line 153
> > 
> >
> > Does it have to run every second? This just checks for errors so a 
> > higher limit should suffice (e.g. 5 seconds).

No, it does not have to run every second. Changed it to every 5 seconds.


- Zameer


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


On Jan. 21, 2016, 2:22 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 21, 2016, 2:22 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is key to operating a large cluster and failure to prune should 
> trigger shutdown to prevent unbounded growth of storage. This patch turns 
> `TaskHistoryPruner` into a service which propagates failure from failed 
> pruning attempts towards the `ServiceManager`. Also completing a TODO which 
> removes a test for behaviour that is very awkward to test for.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
> 735199ac1ab343c24471890aa330d6635c26 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> e2e tests
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[Zameer Manji]

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

(Updated Jan. 21, 2016, 2:55 p.m.)


Review request for Aurora, John Sirois and Maxim Khutornenko.


Changes
---

Maxim's feedback.


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


Repository: aurora


Description
---

Task pruning is key to operating a large cluster and failure to prune should 
trigger shutdown to prevent unbounded growth of storage. This patch turns 
`TaskHistoryPruner` into a service which propagates failure from failed pruning 
attempts towards the `ServiceManager`. Also completing a TODO which removes a 
test for behaviour that is very awkward to test for.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
735199ac1ab343c24471890aa330d6635c26 
  src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
2064089937f5178b1413d386a312f4173a0e35fb 
  src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
295960f13693c6ba0d7075a8ef7f9680a91ae69d 

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


Testing
---

./gradlew build -Pq
e2e tests


Thanks,

Zameer Manji

Re: Review Request 42614: Allowing dual authorizing params to account for thrift API deprecations.

[Aurora ReviewBot]

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

Ship it!


Master (8d3fb24) 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. 21, 2016, 10:05 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42614/
> ---
> 
> (Updated Jan. 21, 2016, 10:05 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> We don't allow dual annotations for authorizing params in thrift. There are 
> cases, however, when it's needed. One example is AURORA-1583 where we would 
> need to support something like below before we cutoff `query` in the next 
> release:
> ```
>   Response killTasks(
>   @AuthorizingParam @Nullable TaskQuery query,
>   @Nullable Lock lock,
>   @AuthorizingParam @Nullable JobKey job,
>   int instances) throws TException;
> ```  
> 
> The new behavior allows at most 2 annotations but only one of the annotated 
> arguments must be non-null during interception.
> 
> Also, added missing test coverage.
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt 6b71fd233af0d137332bc69249d16e433aa198c7 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
>  11d7e465556020571ffeefcf05596e6251ba9d19 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
>  69056624064be4bbd4136afb4dd6e3eb33c5e0d2 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java
>  16a3a3b84c2a06bc340575abb58f853a8f26920d 
> 
> Diff: https://reviews.apache.org/r/42614/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 42614: Allowing dual authorizing params to account for thrift API deprecations.

[Maxim Khutornenko]

> On Jan. 21, 2016, 10:09 p.m., Bill Farner wrote:
> > Before submitting, please verify that end-to-end tests work with this patch.

Verified.


- Maxim


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


On Jan. 21, 2016, 10:05 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42614/
> ---
> 
> (Updated Jan. 21, 2016, 10:05 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> We don't allow dual annotations for authorizing params in thrift. There are 
> cases, however, when it's needed. One example is AURORA-1583 where we would 
> need to support something like below before we cutoff `query` in the next 
> release:
> ```
>   Response killTasks(
>   @AuthorizingParam @Nullable TaskQuery query,
>   @Nullable Lock lock,
>   @AuthorizingParam @Nullable JobKey job,
>   int instances) throws TException;
> ```  
> 
> The new behavior allows at most 2 annotations but only one of the annotated 
> arguments must be non-null during interception.
> 
> Also, added missing test coverage.
> 
> 
> Diffs
> -
> 
>   config/legacy_untested_classes.txt 6b71fd233af0d137332bc69249d16e433aa198c7 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
>  11d7e465556020571ffeefcf05596e6251ba9d19 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
>  69056624064be4bbd4136afb4dd6e3eb33c5e0d2 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java
>  16a3a3b84c2a06bc340575abb58f853a8f26920d 
> 
> Diff: https://reviews.apache.org/r/42614/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[Aurora ReviewBot]

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

Ship it!


Master (b2cc604) 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. 21, 2016, 10:55 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 21, 2016, 10:55 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is key to operating a large cluster and failure to prune should 
> trigger shutdown to prevent unbounded growth of storage. This patch turns 
> `TaskHistoryPruner` into a service which propagates failure from failed 
> pruning attempts towards the `ServiceManager`. Also completing a TODO which 
> removes a test for behaviour that is very awkward to test for.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
> 735199ac1ab343c24471890aa330d6635c26 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> e2e tests
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[John Sirois]

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



src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java (line 
145)


I was envisioning put and take, no schedules, no batching. The queue could 
even be a SynchronousQueue holding ~0 elements. Any reason not to simplify in 
that way?


- John Sirois


On Jan. 21, 2016, 3:55 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 21, 2016, 3:55 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is key to operating a large cluster and failure to prune should 
> trigger shutdown to prevent unbounded growth of storage. This patch turns 
> `TaskHistoryPruner` into a service which propagates failure from failed 
> pruning attempts towards the `ServiceManager`. Also completing a TODO which 
> removes a test for behaviour that is very awkward to test for.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
> 735199ac1ab343c24471890aa330d6635c26 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> e2e tests
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[Maxim Khutornenko]

> On Jan. 21, 2016, 10:50 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java, 
> > line 108
> > 
> >
> > Why BlockingQueue and not something like ConcurrentLinkedQueue? There 
> > is nothing to block here for.
> 
> Zameer Manji wrote:
> `BlockingQueue` has the `drainTo` method which I think makes the 
> iteration nicer.

The price for that is excessive locking and lower perf. You can do the same 
with a faster ConcurrentLinkedQueue and avoid extra collection copying:

```
  FutureTask task;
  while((task = q.poll()) != null) {
task.get();
  }
```


- Maxim


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


On Jan. 21, 2016, 10:55 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 21, 2016, 10:55 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is key to operating a large cluster and failure to prune should 
> trigger shutdown to prevent unbounded growth of storage. This patch turns 
> `TaskHistoryPruner` into a service which propagates failure from failed 
> pruning attempts towards the `ServiceManager`. Also completing a TODO which 
> removes a test for behaviour that is very awkward to test for.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
> 735199ac1ab343c24471890aa330d6635c26 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> e2e tests
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[Zameer Manji]

> On Jan. 21, 2016, 3:20 p.m., John Sirois wrote:
> > src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java, 
> > line 164
> > 
> >
> > I was envisioning put and take, no schedules, no batching. The queue 
> > could even be a SynchronousQueue holding ~0 elements. Any reason not to 
> > simplify in that way?

The documentation for Guava's `AbstractExecutionThreadService` says: "You must 
override the run() method, and it must respond to stop requests.". If we just 
implemented `take`, the thread of the service would block for a long time if 
there was no TaskStateChanges, possibly causing this service to not respond to 
stop requests. I don't know what the implications are of not responding to stop 
requests, but I would like to avoid that case.

I decided that it would be easier to just use `AbstractScheduledService` 
instead and avoid this case.


- Zameer


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


On Jan. 21, 2016, 2:55 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 21, 2016, 2:55 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is key to operating a large cluster and failure to prune should 
> trigger shutdown to prevent unbounded growth of storage. This patch turns 
> `TaskHistoryPruner` into a service which propagates failure from failed 
> pruning attempts towards the `ServiceManager`. Also completing a TODO which 
> removes a test for behaviour that is very awkward to test for.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
> 735199ac1ab343c24471890aa330d6635c26 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> e2e tests
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[Zameer Manji]

> On Jan. 21, 2016, 2:50 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java, 
> > line 108
> > 
> >
> > Why BlockingQueue and not something like ConcurrentLinkedQueue? There 
> > is nothing to block here for.
> 
> Zameer Manji wrote:
> `BlockingQueue` has the `drainTo` method which I think makes the 
> iteration nicer.
> 
> Maxim Khutornenko wrote:
> The price for that is excessive locking and lower perf. You can do the 
> same with a faster ConcurrentLinkedQueue and avoid extra collection copying:
> 
> ```
>   FutureTask task;
>   while((task = q.poll()) != null) {
> task.get();
>   }
> ```

Done.


- Zameer


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


On Jan. 21, 2016, 2:55 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 21, 2016, 2:55 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is key to operating a large cluster and failure to prune should 
> trigger shutdown to prevent unbounded growth of storage. This patch turns 
> `TaskHistoryPruner` into a service which propagates failure from failed 
> pruning attempts towards the `ServiceManager`. Also completing a TODO which 
> removes a test for behaviour that is very awkward to test for.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
> 735199ac1ab343c24471890aa330d6635c26 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> e2e tests
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[Zameer Manji]

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

(Updated Jan. 21, 2016, 3:43 p.m.)


Review request for Aurora, John Sirois and Maxim Khutornenko.


Changes
---

Maxim's feedback.


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


Repository: aurora


Description
---

Task pruning is key to operating a large cluster and failure to prune should 
trigger shutdown to prevent unbounded growth of storage. This patch turns 
`TaskHistoryPruner` into a service which propagates failure from failed pruning 
attempts towards the `ServiceManager`. Also completing a TODO which removes a 
test for behaviour that is very awkward to test for.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
735199ac1ab343c24471890aa330d6635c26 
  src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
2064089937f5178b1413d386a312f4173a0e35fb 
  src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
295960f13693c6ba0d7075a8ef7f9680a91ae69d 

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


Testing
---

./gradlew build -Pq
e2e tests


Thanks,

Zameer Manji

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[Aurora ReviewBot]

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

Ship it!


Master (a2c7ccc) 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. 21, 2016, 11:43 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 21, 2016, 11:43 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is key to operating a large cluster and failure to prune should 
> trigger shutdown to prevent unbounded growth of storage. This patch turns 
> `TaskHistoryPruner` into a service which propagates failure from failed 
> pruning attempts towards the `ServiceManager`. Also completing a TODO which 
> removes a test for behaviour that is very awkward to test for.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
> 735199ac1ab343c24471890aa330d6635c26 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> e2e tests
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[Maxim Khutornenko]

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

Ship it!


Ship It!

- Maxim Khutornenko


On Jan. 21, 2016, 11:43 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 21, 2016, 11:43 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is key to operating a large cluster and failure to prune should 
> trigger shutdown to prevent unbounded growth of storage. This patch turns 
> `TaskHistoryPruner` into a service which propagates failure from failed 
> pruning attempts towards the `ServiceManager`. Also completing a TODO which 
> removes a test for behaviour that is very awkward to test for.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
> 735199ac1ab343c24471890aa330d6635c26 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> e2e tests
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[John Sirois]

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

Ship it!


Ship It!

- John Sirois


On Jan. 21, 2016, 4:43 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 21, 2016, 4:43 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is key to operating a large cluster and failure to prune should 
> trigger shutdown to prevent unbounded growth of storage. This patch turns 
> `TaskHistoryPruner` into a service which propagates failure from failed 
> pruning attempts towards the `ServiceManager`. Also completing a TODO which 
> removes a test for behaviour that is very awkward to test for.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
> 735199ac1ab343c24471890aa330d6635c26 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> e2e tests
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[John Sirois]

> On Jan. 21, 2016, 3:50 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java, 
> > line 108
> > 
> >
> > Why BlockingQueue and not something like ConcurrentLinkedQueue? There 
> > is nothing to block here for.
> 
> Zameer Manji wrote:
> `BlockingQueue` has the `drainTo` method which I think makes the 
> iteration nicer.
> 
> Maxim Khutornenko wrote:
> The price for that is excessive locking and lower perf. You can do the 
> same with a faster ConcurrentLinkedQueue and avoid extra collection copying:
> 
> ```
>   FutureTask task;
>   while((task = q.poll()) != null) {
> task.get();
>   }
> ```
> 
> Zameer Manji wrote:
> Done.

I just want to temper perf concerns with the fact that there are exactly 2 
threads involved here - 1 from EventBus (no AllowConcurrentEvents used here), 
and this 1 polling thread in the main service lifecycle - perf should not 
actually matter here in any reasonable scenario I can think of.

That said - this code reads ~just as clearly so I see no problems with the 
switch.


- John


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


On Jan. 21, 2016, 4:43 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 21, 2016, 4:43 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is key to operating a large cluster and failure to prune should 
> trigger shutdown to prevent unbounded growth of storage. This patch turns 
> `TaskHistoryPruner` into a service which propagates failure from failed 
> pruning attempts towards the `ServiceManager`. Also completing a TODO which 
> removes a test for behaviour that is very awkward to test for.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
> 735199ac1ab343c24471890aa330d6635c26 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> e2e tests
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[John Sirois]

> On Jan. 21, 2016, 4:20 p.m., John Sirois wrote:
> > src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java, 
> > line 164
> > 
> >
> > I was envisioning put and take, no schedules, no batching. The queue 
> > could even be a SynchronousQueue holding ~0 elements. Any reason not to 
> > simplify in that way?
> 
> Zameer Manji wrote:
> The documentation for Guava's `AbstractExecutionThreadService` says: "You 
> must override the run() method, and it must respond to stop requests.". If we 
> just implemented `take`, the thread of the service would block for a long 
> time if there was no TaskStateChanges, possibly causing this service to not 
> respond to stop requests. I don't know what the implications are of not 
> responding to stop requests, but I would like to avoid that case.
> 
> I decided that it would be easier to just use `AbstractScheduledService` 
> instead and avoid this case.

OK - I may follow up with a proposal RB on my own time - I think this can be 
simpler and clearer, but I need to sketch some to see if I'm lying.


- John


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


On Jan. 21, 2016, 4:43 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 21, 2016, 4:43 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is key to operating a large cluster and failure to prune should 
> trigger shutdown to prevent unbounded growth of storage. This patch turns 
> `TaskHistoryPruner` into a service which propagates failure from failed 
> pruning attempts towards the `ServiceManager`. Also completing a TODO which 
> removes a test for behaviour that is very awkward to test for.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
> 735199ac1ab343c24471890aa330d6635c26 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> e2e tests
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Review Request 42628: Add storage API methods for fetching amd mutating a task by ID.

[Bill Farner]

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

Review request for Aurora, John Sirois and Maxim Khutornenko.


Repository: aurora


Description
---

This is partially in response to some discussion on other reviews.  `fetchTask` 
makes several call sites much more natural (not having to possibly deal with 
multiple results), and `mutateTask` actually represents all but 2 call sites 
that modifies tasks.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/http/StructDump.java 
4fa5254457b40feb4bb8b6512c09dd4946935b64 
  
src/main/java/org/apache/aurora/scheduler/scheduling/RescheduleCalculator.java 
c136d1abc82b0f91f2b148e998e30b6b50856d51 
  src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 
720b5e5d14a66412dceaf9d9954c7f5aa01d495c 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
610915820b50a73f6757ad60a0e35e3f641f7fdf 
  src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 
62639c4879ecd652f4d51a94159f128997c03df5 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
d4061340dc4d9975b6783b6cb38b457632bd4bb0 
  src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
c44ff47d7dffc0880bcdb84ed26f53bcb7d45e0e 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
c55dcc94a4ec080fedf85ee792c968e8f119f53a 
  
src/test/java/org/apache/aurora/scheduler/scheduling/RescheduleCalculatorImplTest.java
 b380f21ac169b4991158f39dc70526e11fca54f0 
  src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 
6d4268956acdf85465a3d05191456c74bc426998 
  
src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java 
a6bfc7ac91891330600bd260cc8efff1240007e1 
  src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
5a9b6c1030a6db2190d34bf531deb99dfd8df672 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
430527051df7cd7026ae2754f5a9616e39857468 
  
src/test/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java 
bf344a4ddbc7f294f938df67f27bfdb32cf37457 

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


Testing
---


Thanks,

Bill Farner

Re: Review Request 42628: Add storage API methods for fetching amd mutating a task by ID.

[Bill Farner]

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


Related discussion, i would like to evaluate removing this function:
```
ImmutableSet mutateTasks(
Query.Builder query,
Function mutator);
```

The function itself is only called in `StorageBackfill`, and could be replaced 
by a query + selective mutations.

- Bill Farner


On Jan. 21, 2016, 5:25 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42628/
> ---
> 
> (Updated Jan. 21, 2016, 5:25 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is partially in response to some discussion on other reviews.  
> `fetchTask` makes several call sites much more natural (not having to 
> possibly deal with multiple results), and `mutateTask` actually represents 
> all but 2 call sites that modifies tasks.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/StructDump.java 
> 4fa5254457b40feb4bb8b6512c09dd4946935b64 
>   
> src/main/java/org/apache/aurora/scheduler/scheduling/RescheduleCalculator.java
>  c136d1abc82b0f91f2b148e998e30b6b50856d51 
>   src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 
> 720b5e5d14a66412dceaf9d9954c7f5aa01d495c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 610915820b50a73f6757ad60a0e35e3f641f7fdf 
>   src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 
> 62639c4879ecd652f4d51a94159f128997c03df5 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> d4061340dc4d9975b6783b6cb38b457632bd4bb0 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
> c44ff47d7dffc0880bcdb84ed26f53bcb7d45e0e 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
> c55dcc94a4ec080fedf85ee792c968e8f119f53a 
>   
> src/test/java/org/apache/aurora/scheduler/scheduling/RescheduleCalculatorImplTest.java
>  b380f21ac169b4991158f39dc70526e11fca54f0 
>   src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 
> 6d4268956acdf85465a3d05191456c74bc426998 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java
>  a6bfc7ac91891330600bd260cc8efff1240007e1 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 5a9b6c1030a6db2190d34bf531deb99dfd8df672 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 430527051df7cd7026ae2754f5a9616e39857468 
>   
> src/test/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java
>  bf344a4ddbc7f294f938df67f27bfdb32cf37457 
> 
> Diff: https://reviews.apache.org/r/42628/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 42628: Add storage API methods for fetching amd mutating a task by ID.

[Aurora ReviewBot]

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


Master (a2c7ccc) is red with this patch.
  ./build-support/jenkins/build.sh

   self._clock.tick(0.5 + epsilon)
   
self._clock.converge(threads=[hct.threaded_health_checker])
   
self._clock.assert_waiting(hct.threaded_health_checker, amount=1)  # 
interval_secs
   assert hct._total_latency == 0.5
   assert 
hct.metrics.sample()['total_latency_secs'] == 0.5
   assert hct.metrics.sample()['checks'] == 1
 
   # tick again
   self._clock.tick(1.0 + epsilon)
   
self._clock.converge(threads=[hct.threaded_health_checker])
   self._clock.tick(0.5 + epsilon)
   
self._clock.converge(threads=[hct.threaded_health_checker])
   
self._clock.assert_waiting(hct.threaded_health_checker, amount=1)  # 
interval_secs
 > assert hct._total_latency == 1.0
 E AssertionError: assert 1.0036 
== 1.0
 E  +  where 1.0036 = 
._total_latency
 
 
src/test/python/apache/aurora/executor/common/test_health_checker.py:184: 
AssertionError
 -- Captured stderr call --
 [] Time now: 0.0
 [] Time now: 0.0
 [] Time now: 1.0
 [] Time now: 1.001
 [] Time now: 1.001
 [] Time now: 1.5
 [] Time now: 1.502
 [] Time now: 1.502
 [] Time now: 2.5
 [] Time now: 2.503
 [] Time now: 2.503
 [] Time now: 3.0
 [] Time now: 3.004
  generated xml file: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/test-results/src.test.python.apache.aurora.executor.common.common.xml
 
 = 1 failed, 42 passed, 2 skipped in 3.71 seconds 
=
 
FAILURE


01:35:39 05:04   [complete]
   FAILURE


I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Jan. 22, 2016, 1:25 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42628/
> ---
> 
> (Updated Jan. 22, 2016, 1:25 a.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is partially in response to some discussion on other reviews.  
> `fetchTask` makes several call sites much more natural (not having to 
> possibly deal with multiple results), and `mutateTask` actually represents 
> all but 2 call sites that modifies tasks.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/StructDump.java 
> 4fa5254457b40feb4bb8b6512c09dd4946935b64 
>   
> src/main/java/org/apache/aurora/scheduler/scheduling/RescheduleCalculator.java
>  c136d1abc82b0f91f2b148e998e30b6b50856d51 
>   src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 
> 720b5e5d14a66412dceaf9d9954c7f5aa01d495c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 610915820b50a73f6757ad60a0e35e3f641f7fdf 
>   src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 
> 62639c4879ecd652f4d51a94159f128997c03df5 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> d4061340dc4d9975b6783b6cb38b457632bd4bb0 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
> c44ff47d7dffc0880bcdb84ed26f53bcb7d45e0e 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
> c55dcc94a4ec080fedf85ee792c968e8f119f53a 
>   
> src/test/java/org/apache/aurora/scheduler/scheduling/RescheduleCalculatorImplTest.java
>  b380f21ac169b4991158f39dc70526e11fca54f0 
>   src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 
> 6d4268956acdf85465a3d05191456c74bc426998 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java
>  a6bfc7ac91891330600bd260cc8efff1240007e1 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 5a9b6c1030a6db2190d34bf531deb99dfd8df672 
>   src/t

Re: Review Request 42628: Add storage API methods for fetching amd mutating a task by ID.

[Bill Farner]

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


@ReviewBot retry

- Bill Farner


On Jan. 21, 2016, 5:25 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42628/
> ---
> 
> (Updated Jan. 21, 2016, 5:25 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is partially in response to some discussion on other reviews.  
> `fetchTask` makes several call sites much more natural (not having to 
> possibly deal with multiple results), and `mutateTask` actually represents 
> all but 2 call sites that modifies tasks.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/StructDump.java 
> 4fa5254457b40feb4bb8b6512c09dd4946935b64 
>   
> src/main/java/org/apache/aurora/scheduler/scheduling/RescheduleCalculator.java
>  c136d1abc82b0f91f2b148e998e30b6b50856d51 
>   src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 
> 720b5e5d14a66412dceaf9d9954c7f5aa01d495c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 610915820b50a73f6757ad60a0e35e3f641f7fdf 
>   src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 
> 62639c4879ecd652f4d51a94159f128997c03df5 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> d4061340dc4d9975b6783b6cb38b457632bd4bb0 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
> c44ff47d7dffc0880bcdb84ed26f53bcb7d45e0e 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
> c55dcc94a4ec080fedf85ee792c968e8f119f53a 
>   
> src/test/java/org/apache/aurora/scheduler/scheduling/RescheduleCalculatorImplTest.java
>  b380f21ac169b4991158f39dc70526e11fca54f0 
>   src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 
> 6d4268956acdf85465a3d05191456c74bc426998 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java
>  a6bfc7ac91891330600bd260cc8efff1240007e1 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 5a9b6c1030a6db2190d34bf531deb99dfd8df672 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 430527051df7cd7026ae2754f5a9616e39857468 
>   
> src/test/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java
>  bf344a4ddbc7f294f938df67f27bfdb32cf37457 
> 
> Diff: https://reviews.apache.org/r/42628/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 42628: Add storage API methods for fetching amd mutating a task by ID.

[Aurora ReviewBot]

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

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, 1:25 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42628/
> ---
> 
> (Updated Jan. 22, 2016, 1:25 a.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is partially in response to some discussion on other reviews.  
> `fetchTask` makes several call sites much more natural (not having to 
> possibly deal with multiple results), and `mutateTask` actually represents 
> all but 2 call sites that modifies tasks.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/StructDump.java 
> 4fa5254457b40feb4bb8b6512c09dd4946935b64 
>   
> src/main/java/org/apache/aurora/scheduler/scheduling/RescheduleCalculator.java
>  c136d1abc82b0f91f2b148e998e30b6b50856d51 
>   src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 
> 720b5e5d14a66412dceaf9d9954c7f5aa01d495c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 610915820b50a73f6757ad60a0e35e3f641f7fdf 
>   src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 
> 62639c4879ecd652f4d51a94159f128997c03df5 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> d4061340dc4d9975b6783b6cb38b457632bd4bb0 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
> c44ff47d7dffc0880bcdb84ed26f53bcb7d45e0e 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
> c55dcc94a4ec080fedf85ee792c968e8f119f53a 
>   
> src/test/java/org/apache/aurora/scheduler/scheduling/RescheduleCalculatorImplTest.java
>  b380f21ac169b4991158f39dc70526e11fca54f0 
>   src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 
> 6d4268956acdf85465a3d05191456c74bc426998 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java
>  a6bfc7ac91891330600bd260cc8efff1240007e1 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 5a9b6c1030a6db2190d34bf531deb99dfd8df672 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 430527051df7cd7026ae2754f5a9616e39857468 
>   
> src/test/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java
>  bf344a4ddbc7f294f938df67f27bfdb32cf37457 
> 
> Diff: https://reviews.apache.org/r/42628/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 42628: Add storage API methods for fetching amd mutating a task by ID.

[Maxim Khutornenko]

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

Ship it!


Ship It!

- Maxim Khutornenko


On Jan. 22, 2016, 1:25 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42628/
> ---
> 
> (Updated Jan. 22, 2016, 1:25 a.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is partially in response to some discussion on other reviews.  
> `fetchTask` makes several call sites much more natural (not having to 
> possibly deal with multiple results), and `mutateTask` actually represents 
> all but 2 call sites that modifies tasks.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/StructDump.java 
> 4fa5254457b40feb4bb8b6512c09dd4946935b64 
>   
> src/main/java/org/apache/aurora/scheduler/scheduling/RescheduleCalculator.java
>  c136d1abc82b0f91f2b148e998e30b6b50856d51 
>   src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 
> 720b5e5d14a66412dceaf9d9954c7f5aa01d495c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 610915820b50a73f6757ad60a0e35e3f641f7fdf 
>   src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 
> 62639c4879ecd652f4d51a94159f128997c03df5 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> d4061340dc4d9975b6783b6cb38b457632bd4bb0 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
> c44ff47d7dffc0880bcdb84ed26f53bcb7d45e0e 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
> c55dcc94a4ec080fedf85ee792c968e8f119f53a 
>   
> src/test/java/org/apache/aurora/scheduler/scheduling/RescheduleCalculatorImplTest.java
>  b380f21ac169b4991158f39dc70526e11fca54f0 
>   src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 
> 6d4268956acdf85465a3d05191456c74bc426998 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java
>  a6bfc7ac91891330600bd260cc8efff1240007e1 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 5a9b6c1030a6db2190d34bf531deb99dfd8df672 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 430527051df7cd7026ae2754f5a9616e39857468 
>   
> src/test/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java
>  bf344a4ddbc7f294f938df67f27bfdb32cf37457 
> 
> Diff: https://reviews.apache.org/r/42628/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 42565: Remove support for adding guice modules via command line arguments.

[Zameer Manji]

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


My primary objection to this review is to how it changes the security 
configuration. I'm +1 to the idea of not accepting modules on the CLI for the 
reasons specified in the review summary and because I think accepting a bunch 
of modules blindly is a lot of fire power just for extending behaviour. Users 
who need to do that might be better served with more precise extention points.

Bill, I don't think anyone uses the `extra_modules` argument, git log doesn't 
indicate a really good reason for introducing the feautre, and I think blindly 
accepting a bunch of modules to extend behaviour is not the way to go. If you 
would like, feel free to peel that out into a seperate review so it for sure 
can make it into 0.12.0.


src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
 (line 77)


Just to be clear, this command line argument will only accept two modules, 
the ini realm module and the kerberos realm module?

This means users who need to setup a custom realm (probably any medium to 
large cluster operator), must create a custom main.

I'm unsure if this is the ideal approach to take w.r.t security. I think 
this requires some discussion on the mailing list. For example, an alternative 
to what you have done here would be to remove this argument entirely, and 
request users specify realms in the ini file (see 
http://shiro.apache.org/configuration.html section "INI Sections").


- Zameer Manji


On Jan. 20, 2016, 12:21 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42565/
> ---
> 
> (Updated Jan. 20, 2016, 12:21 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This patch proposes that users installing custom modules do so via a custom 
> main.
> 
> Specifying custom modules on the command line has proven troublesome for 
> replacing the command line args system with one where all arguments are 
> injected and explicitly-defined.  It also adds complexity to the scheduler 
> application by unnecessarily punching holes at specific places.
> 
> If this proposal is agreeable, i will add a NEWS entry and document how one 
> might implement a custom main to add modules.  The tl;dr, however, is to 
> invoke `SchedulerMain.publicMain(customModule)`
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/MoreModules.java 
> d5f96543d1068bf30b9d173a247c2806faf35578 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> 0659c358479283756179c2cabebc8416730cc3e3 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  e32862034a1ad47dae8fff89cb04deb34ccd90e2 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/ModuleParser.java 
> ccd9a20e8b18831458cba2d53e6b8b84fef06162 
>   src/test/java/org/apache/aurora/scheduler/app/MoreModulesTest.java 
> b2fb3c9dcba64f69a05894f506ba43766f74ddaa 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  3e811a4f4d2c82892217ca1f950ac792303fbcf3 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ModuleParserTest.java
>  baaeb2390a909de1a92e4328d35a49f7b74c36cb 
> 
> Diff: https://reviews.apache.org/r/42565/diff/
> 
> 
> Testing
> ---
> 
> end-to-end tests, `./gradlew run`
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

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

[John Sirois]

---
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 42332: Turn TaskHistoryPruner into a service and trigger shutdown on pruning failure.

[John Sirois]

> On Jan. 21, 2016, 4:20 p.m., John Sirois wrote:
> > src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java, 
> > line 164
> > 
> >
> > I was envisioning put and take, no schedules, no batching. The queue 
> > could even be a SynchronousQueue holding ~0 elements. Any reason not to 
> > simplify in that way?
> 
> Zameer Manji wrote:
> The documentation for Guava's `AbstractExecutionThreadService` says: "You 
> must override the run() method, and it must respond to stop requests.". If we 
> just implemented `take`, the thread of the service would block for a long 
> time if there was no TaskStateChanges, possibly causing this service to not 
> respond to stop requests. I don't know what the implications are of not 
> responding to stop requests, but I would like to avoid that case.
> 
> I decided that it would be easier to just use `AbstractScheduledService` 
> instead and avoid this case.
> 
> John Sirois wrote:
> OK - I may follow up with a proposal RB on my own time - I think this can 
> be simpler and clearer, but I need to sketch some to see if I'm lying.

See what you think of this: https://reviews.apache.org/r/42639/


- John


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


On Jan. 21, 2016, 4:43 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42332/
> ---
> 
> (Updated Jan. 21, 2016, 4:43 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1582
> https://issues.apache.org/jira/browse/AURORA-1582
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Task pruning is key to operating a large cluster and failure to prune should 
> trigger shutdown to prevent unbounded growth of storage. This patch turns 
> `TaskHistoryPruner` into a service which propagates failure from failed 
> pruning attempts towards the `ServiceManager`. Also completing a TODO which 
> removes a test for behaviour that is very awkward to test for.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/pruning/PruningModule.java 
> 735199ac1ab343c24471890aa330d6635c26 
>   src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 
> 2064089937f5178b1413d386a312f4173a0e35fb 
>   
> src/test/java/org/apache/aurora/scheduler/pruning/TaskHistoryPrunerTest.java 
> 295960f13693c6ba0d7075a8ef7f9680a91ae69d 
> 
> Diff: https://reviews.apache.org/r/42332/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> e2e tests
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

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.

[Maxim Khutornenko]

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


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.

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

[Maxim Khutornenko]

---
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 42628: Add storage API methods for fetching amd mutating a task by ID.

[John Sirois]

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

Ship it!


Ship It!

- John Sirois


On Jan. 21, 2016, 6:25 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42628/
> ---
> 
> (Updated Jan. 21, 2016, 6:25 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This is partially in response to some discussion on other reviews.  
> `fetchTask` makes several call sites much more natural (not having to 
> possibly deal with multiple results), and `mutateTask` actually represents 
> all but 2 call sites that modifies tasks.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/StructDump.java 
> 4fa5254457b40feb4bb8b6512c09dd4946935b64 
>   
> src/main/java/org/apache/aurora/scheduler/scheduling/RescheduleCalculator.java
>  c136d1abc82b0f91f2b148e998e30b6b50856d51 
>   src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 
> 720b5e5d14a66412dceaf9d9954c7f5aa01d495c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 610915820b50a73f6757ad60a0e35e3f641f7fdf 
>   src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 
> 62639c4879ecd652f4d51a94159f128997c03df5 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> d4061340dc4d9975b6783b6cb38b457632bd4bb0 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
> c44ff47d7dffc0880bcdb84ed26f53bcb7d45e0e 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
> c55dcc94a4ec080fedf85ee792c968e8f119f53a 
>   
> src/test/java/org/apache/aurora/scheduler/scheduling/RescheduleCalculatorImplTest.java
>  b380f21ac169b4991158f39dc70526e11fca54f0 
>   src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 
> 6d4268956acdf85465a3d05191456c74bc426998 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractCronJobStoreTest.java
>  a6bfc7ac91891330600bd260cc8efff1240007e1 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 5a9b6c1030a6db2190d34bf531deb99dfd8df672 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 430527051df7cd7026ae2754f5a9616e39857468 
>   
> src/test/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java
>  bf344a4ddbc7f294f938df67f27bfdb32cf37457 
> 
> Diff: https://reviews.apache.org/r/42628/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

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

Re: Review Request 42565: Remove support for adding guice modules via command line arguments.

[Bill Farner]

> On Jan. 21, 2016, 6:44 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java,
> >  line 80
> > 
> >
> > Just to be clear, this command line argument will only accept two 
> > modules, the ini realm module and the kerberos realm module?
> > 
> > This means users who need to setup a custom realm (probably any medium 
> > to large cluster operator), must create a custom main.
> > 
> > I'm unsure if this is the ideal approach to take w.r.t security. I 
> > think this requires some discussion on the mailing list. For example, an 
> > alternative to what you have done here would be to remove this argument 
> > entirely, and request users specify realms in the ini file (see 
> > http://shiro.apache.org/configuration.html section "INI Sections").

In case the concern about a custom main is that it sounds dautning, here's an 
illustration:

```
public class CustomMain {
  public static void main(String[] args) {
SchedulerMain.applyStaticArgumentValues(args);
SchedulerMain.publicMain(new CustomRealmModule());
  }
}
```

(It may be worth building in the `applyStaticArgumentValues` step, so we can 
reduce even further.)

I see 2 upsides with this:
- The API for customization (whether security or other arbitrary feature 
addition) is uniform.
- The `CustomMain` can use the same commane line arguments system as the rest 
of the scheduler (this will work with the forthcoming replacement as well).

I believe your suggestion to use an INI section falls apart in 2 ways:
- the `[main]` section only applies when using pure INI configuration for 
shiro. I believe we fall under the programmatic configuration section, with 
shiro-guice doing the programmatic bits.  The ini file aurora accepts is read 
by `org.apache.shiro.realm.text.IniRealm` and it only uses the `users` and 
`roles` sections (see 
https://shiro.apache.org/static/1.2.3/apidocs/org/apache/shiro/realm/text/IniRealm.html)
- i don't believe classes configured with pure shiro INI configuration would 
participate in guice injection

I'm open to ideas here.  Ultimately i'm trying to preserve integration of 
custom secruity controls with features that exist today:
1. support for participation in guice injection
2. support for participation in the same configuration system as the rest of 
the scheduler

If (1) is not necessary, that simplifies things.  An alternative to (2) is to 
force custom code to handle its own configuration (e.g. with environment 
variables).


- Bill


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


On Jan. 20, 2016, 12:21 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42565/
> ---
> 
> (Updated Jan. 20, 2016, 12:21 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This patch proposes that users installing custom modules do so via a custom 
> main.
> 
> Specifying custom modules on the command line has proven troublesome for 
> replacing the command line args system with one where all arguments are 
> injected and explicitly-defined.  It also adds complexity to the scheduler 
> application by unnecessarily punching holes at specific places.
> 
> If this proposal is agreeable, i will add a NEWS entry and document how one 
> might implement a custom main to add modules.  The tl;dr, however, is to 
> invoke `SchedulerMain.publicMain(customModule)`
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/MoreModules.java 
> d5f96543d1068bf30b9d173a247c2806faf35578 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> 0659c358479283756179c2cabebc8416730cc3e3 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  e32862034a1ad47dae8fff89cb04deb34ccd90e2 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/ModuleParser.java 
> ccd9a20e8b18831458cba2d53e6b8b84fef06162 
>   src/test/java/org/apache/aurora/scheduler/app/MoreModulesTest.java 
> b2fb3c9dcba64f69a05894f506ba43766f74ddaa 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  3e811a4f4d2c82892217ca1f950ac792303fbcf3 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ModuleParserTest.java
>  baaeb2390a909de1a92e4328d35a49f7b74c36cb 
> 
> Diff: https://reviews.apache.org/r/42565/diff/
> 
> 
> Testing
> ---
> 
> end-to-end tests, `./gradlew run`
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 42565: Remove support for adding guice modules via command line arguments.

[Bill Farner]

> On Jan. 21, 2016, 6:44 p.m., Zameer Manji wrote:
> > My primary objection to this review is to how it changes the security 
> > configuration. I'm +1 to the idea of not accepting modules on the CLI for 
> > the reasons specified in the review summary and because I think accepting a 
> > bunch of modules blindly is a lot of fire power just for extending 
> > behaviour. Users who need to do that might be better served with more 
> > precise extention points.
> > 
> > Bill, I don't think anyone uses the `extra_modules` argument, git log 
> > doesn't indicate a really good reason for introducing the feautre, and I 
> > think blindly accepting a bunch of modules to extend behaviour is not the 
> > way to go. If you would like, feel free to peel that out into a seperate 
> > review so it for sure can make it into 0.12.0.

I've gone ahead and split the `-extra_modules` bit into 
https://reviews.apache.org/r/42565/ so we can focus on the shiro side here.


- Bill


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


On Jan. 20, 2016, 12:21 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42565/
> ---
> 
> (Updated Jan. 20, 2016, 12:21 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This patch proposes that users installing custom modules do so via a custom 
> main.
> 
> Specifying custom modules on the command line has proven troublesome for 
> replacing the command line args system with one where all arguments are 
> injected and explicitly-defined.  It also adds complexity to the scheduler 
> application by unnecessarily punching holes at specific places.
> 
> If this proposal is agreeable, i will add a NEWS entry and document how one 
> might implement a custom main to add modules.  The tl;dr, however, is to 
> invoke `SchedulerMain.publicMain(customModule)`
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/app/MoreModules.java 
> d5f96543d1068bf30b9d173a247c2806faf35578 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> 0659c358479283756179c2cabebc8416730cc3e3 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  e32862034a1ad47dae8fff89cb04deb34ccd90e2 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/ModuleParser.java 
> ccd9a20e8b18831458cba2d53e6b8b84fef06162 
>   src/test/java/org/apache/aurora/scheduler/app/MoreModulesTest.java 
> b2fb3c9dcba64f69a05894f506ba43766f74ddaa 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  3e811a4f4d2c82892217ca1f950ac792303fbcf3 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ModuleParserTest.java
>  baaeb2390a909de1a92e4328d35a49f7b74c36cb 
> 
> Diff: https://reviews.apache.org/r/42565/diff/
> 
> 
> Testing
> ---
> 
> end-to-end tests, `./gradlew run`
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Review Request 42645: Remove scheduler flag -extra_modules.

[Bill Farner]

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

Review request for Aurora and Zameer Manji.


Repository: aurora


Description
---

Split out of https://reviews.apache.org/r/42565/


Diffs
-

  NEWS 37a7a7933b20e112416b01260d97fc2f47c24027 
  src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
0659c358479283756179c2cabebc8416730cc3e3 

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


Testing
---


Thanks,

Bill Farner

Re: Review Request 42645: Remove scheduler flag -extra_modules.

[Aurora ReviewBot]

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


Master (66a4d5f) is green with this patch.
  ./build-support/jenkins/build.sh

However, it appears that it might lack test coverage.

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Jan. 22, 2016, 7:02 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42645/
> ---
> 
> (Updated Jan. 22, 2016, 7:02 a.m.)
> 
> 
> Review request for Aurora and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Split out of https://reviews.apache.org/r/42565/
> 
> 
> Diffs
> -
> 
>   NEWS 37a7a7933b20e112416b01260d97fc2f47c24027 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> 0659c358479283756179c2cabebc8416730cc3e3 
> 
> Diff: https://reviews.apache.org/r/42645/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Review Request 42646: Remove storage backfill and TaskStore mutateTasks.

[Bill Farner]

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

Review request for Aurora, John Sirois and Maxim Khutornenko.


Repository: aurora


Description
---

Related to a comment i made in https://reviews.apache.org/r/42628/, 
StorageBackfill was the only call site for `Storage.Mutable.mutateTasks`.  As 
you can see from the patch, there's a lot that crumbles with it.

Since it doesn't show in the patch, StorageBackfill was only filling the 
`TaskConfig.job` field in tasks and cron jobs:
https://github.com/apache/aurora/blob/9ed81a7db58f6a7cb308c8ac6a545705351c8c0e/src/main/java/org/apache/aurora/scheduler/storage/StorageBackfill.java#L72

This backfill has been in place since `0.6.0-incubating` and should be safe to 
remove.
https://github.com/apache/aurora/commit/18ae0ab9696e3565cf57f6a2550c61142e76bee5#diff-74fad6db735428c9b72017b9097bb0c1L124


Diffs
-

  src/main/java/org/apache/aurora/scheduler/SchedulerLifecycle.java 
0d3874e9632952c54ef5ae9aba78638e47c87056 
  src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
376f48545eb860aad5afa028d3b96d04acc08377 
  
src/main/java/org/apache/aurora/scheduler/storage/CallOrderEnforcingStorage.java
 de4ada431634fb171fab109f1923da810b361205 
  src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
578bb37de8853c4228e76b31f601430b7170946a 
  src/main/java/org/apache/aurora/scheduler/storage/StorageBackfill.java 
609a6242e8918bf4a044e6e5e2c8dfe4b900192e 
  src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 
4e4f8d2c0f237c4480abe101835176f7d69958db 
  src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
43fda1d001f52f916b8a6d75fcdb74d4280eaa41 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
042f71dd8bc81d03f2759ee7f7f4b65a098d11e2 
  src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
728353116c8892c015a6443d8db09ba241b32230 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
8fd024a460cd948dab703b99788b1ed2d6c43bc3 
  src/test/java/org/apache/aurora/scheduler/SchedulerLifecycleTest.java 
bab45670d66dfed23dd6e0339687166c9be44ba4 
  src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
9fb8aad5d1c0412efc6d1176e543321ebe503e03 
  
src/test/java/org/apache/aurora/scheduler/app/local/FakeNonVolatileStorage.java 
0768ec37bbc6c3c101aa04a953a36a4af7b25963 
  src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
1ac41d18fd9d603c4c342f9635e116bfd055f858 
  src/test/java/org/apache/aurora/scheduler/storage/StorageBackfillTest.java 
2570464c85630a55d3e6c8653fc0307d54504e15 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
7382eca281eeab17d407ed140f16d6a633d8ad72 
  
src/test/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorageTest.java
 3c4e2bd5eefc141a5d2c5d0e56881899816652f8 

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


Testing
---


Thanks,

Bill Farner