Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

> On Sept. 15, 2016, 10:58 a.m., Joshua Cohen wrote: > > src/main/java/org/apache/aurora/scheduler/BatchWorker.java, lines 166-167 > > > > > > super nitpicky: mind swapping the order of these args to keep inline >

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/ --- (Updated Sept. 16, 2016, 8:33 p.m.) Review request for Aurora, Joshua Cohen,

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

> On Sept. 15, 2016, 10:58 a.m., Joshua Cohen wrote: > > src/main/java/org/apache/aurora/scheduler/BatchWorker.java, lines 166-167 > > > > > > super nitpicky: mind swapping the order of these args to keep inline >

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/#review149256 --- Ship it! Ship It! - Zameer Manji On Sept. 16, 2016, 1:03

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

> On Sept. 16, 2016, 6:18 p.m., Zameer Manji wrote: > > src/main/java/org/apache/aurora/scheduler/BatchWorker.java, line 213 > > > > > > I think this is a misuse of `CompletalbleFuture` I discovered this as I > >

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/#review149227 --- src/main/java/org/apache/aurora/scheduler/BatchWorker.java (line

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/#review149044 --- Ship it! Overall lgtm. Agree w/ Zameer that we should ship all

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/#review148994 --- Ship it! Master (5069f93) is green with this patch.

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

> On Sept. 14, 2016, 6:36 p.m., Zameer Manji wrote: > > This patch LGTM. Just a single concern here and some questions. I will be > > moving on to the subsequent patches shortly. > > Please do not commit this until all parts are shipped. Also, please don't > > forget to rebase/update

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

> On Sept. 14, 2016, 11:36 a.m., Zameer Manji wrote: > > This patch LGTM. Just a single concern here and some questions. I will be > > moving on to the subsequent patches shortly. > > Please do not commit this until all parts are shipped. Also, please don't > > forget to rebase/update

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/ --- (Updated Sept. 14, 2016, 10:41 p.m.) Review request for Aurora, Joshua Cohen,

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

> On Sept. 14, 2016, 6:36 p.m., Zameer Manji wrote: > > This patch LGTM. Just a single concern here and some questions. I will be > > moving on to the subsequent patches shortly. > > Please do not commit this until all parts are shipped. Also, please don't > > forget to rebase/update

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/#review148950 --- Fix it, then Ship it! This patch LGTM. Just a single concern

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

> On Sept. 13, 2016, 6:29 p.m., Zameer Manji wrote: > > src/main/java/org/apache/aurora/scheduler/BatchWorker.java, line 110 > > > > > > The documentation implies we are returning a boolean but we are > > returning

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/#review148936 --- Master (5069f93) is red with this patch.

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/ --- (Updated Sept. 14, 2016, 5:25 p.m.) Review request for Aurora, Joshua Cohen,

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/#review148931 --- Ship it! Master (5069f93) is green with this patch.

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

> On Sept. 14, 2016, 4:56 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/scheduling/TaskThrottler.java, > > lines 76-90 > > > > > > This code is exposing a slight design smell in my eye: We

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

> On Sept. 14, 2016, 6:56 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/scheduling/TaskThrottler.java, > > lines 76-90 > > > > > > This code is exposing a slight design smell in my eye: We

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/#review148918 --- Fix it, then Ship it! LGTM. A couple of smaller points below,

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/ --- (Updated Sept. 14, 2016, 4:47 p.m.) Review request for Aurora, Joshua Cohen,

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

> On Sept. 14, 2016, 1:29 a.m., Zameer Manji wrote: > > src/main/java/org/apache/aurora/scheduler/BatchWorker.java, line 110 > > > > > > The documentation implies we are returning a boolean but we are > > returning

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/#review148837 --- src/main/java/org/apache/aurora/scheduler/BatchWorker.java (line

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/#review148823 --- Ship it! Master (5069f93) is green with this patch.

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/ --- (Updated Sept. 14, 2016, 12:16 a.m.) Review request for Aurora, Joshua Cohen,

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

> On Sept. 11, 2016, 11:02 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/BatchWorker.java, lines 257-259 > > > > > > 1) Doesn't that effectively result in a busy loop where we are queuing >

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/#review148806 --- Ship it! Master (633948a) is green with this patch.

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/ --- (Updated Sept. 13, 2016, 11:14 p.m.) Review request for Aurora, Joshua Cohen,

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

> On Sept. 12, 2016, 1:02 a.m., Stephan Erb wrote: > > Thanks for the excellent write up and the time you took to split the review > > into sizable chunks. A full review will take some time. I will start with a > > couple of highlevel questions. > > > > a) Do you have some results of the jmh

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

> On Sept. 13, 2016, 1:14 a.m., Zameer Manji wrote: > > src/main/java/org/apache/aurora/scheduler/BatchWorker.java, line 188 > > > > > > Our current metrics use 'nanos' to indicate nanoseconds instead of > > 'ns'.

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/#review148633 --- I noticed that `CompletableFuture` might be suitable here. I will

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/#review148625 --- Ship it! Master (c4903d8) is green with this patch.

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/ --- (Updated Sept. 12, 2016, 10:51 p.m.) Review request for Aurora, Joshua Cohen,

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

> On Sept. 12, 2016, 6:26 p.m., Joshua Cohen wrote: > > Along the lines of the question Stephan raised, what happens in the event > > of a failover mid-batch, especially w.r.t. repeatable work? Same as today: the transaction either happens or does not. In case of a cron job (the only user of

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

> On Sept. 11, 2016, 11:02 p.m., Stephan Erb wrote: > > Thanks for the excellent write up and the time you took to split the review > > into sizable chunks. A full review will take some time. I will start with a > > couple of highlevel questions. > > > > a) Do you have some results of the jmh

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/#review148433 --- Thanks for the excellent write up and the time you took to split

Re: Review Request 51759: Batching writes - Part 1 (of 3): Introducing BatchWorker and task event batching.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51759/#review148349 --- Ship it! Master (c7f710a) is green with this patch.