> On May 4, 2015, 9:56 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java, line 156
> > <https://reviews.apache.org/r/33689/diff/1/?file=947578#file947578line156>
> >
> >     why "-1"?

Because there is already 1 item in the queue :)


> On May 4, 2015, 9:56 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java, lines 
> > 199-203
> > <https://reviews.apache.org/r/33689/diff/1/?file=947578#file947578line199>
> >
> >     This isn't necessary. You can use `.execute(...)` instead and rely on a 
> > single threaded executor to enforce sequential item processing.
> 
> Ben Mahler wrote:
>     You mean the "same thread" executor, yes? Single threaded executor is 
> still asynchronous IIUC.
>     
>     I was a bit hesitant to have the code assume a certain implementation of 
> ExecutorService, seems brittle? If the implementation changes then the 
> throughput can regress substantially.
> 
> Maxim Khutornenko wrote:
>     No, I meant the async executor with a single thread in it. Actually, 
> thinking more about perf here, I think this approach may result in subpar 
> concurrency due to draining updates "too fast" and thus never reaching the 
> max batch size. 
>     
>     What are your thoughts about dropping the thread-isolation requirement 
> completely and implementing batch processing directly on a service thread? We 
> would have to handle RuntimeExceptions explicitly to isolate single batch 
> failures but that should still be easier to reason than "a single-threaded 
> executor within a serial service thread loop".

Maxim and I chatted offline, for posterity: the purpose of batching is to 
accumulate work while processing a batch, which no longer occurs once we don't 
wait like this, so the throughput tanks as expected unless one uses the same 
thread executor. We'll drop the executor complexity altogether and just catch 
exceptions here.


> On May 4, 2015, 9:56 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/SchedulerModule.java, line 93
> > <https://reviews.apache.org/r/33689/diff/1/?file=947577#file947577line93>
> >
> >     Register with lifecycle instead, e.g.:
> >     ```
> >     
> > SchedulerServicesModule.addSchedulerActiveServiceBinding(binder()).to(UserTaskLauncher.class);
> >     ```

Done.


> On May 4, 2015, 9:56 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/SchedulerModule.java, line 82
> > <https://reviews.apache.org/r/33689/diff/1/?file=947577#file947577line82>
> >
> >     Remove the TODO.

This has gone away now that we no longer use an executor.


> On May 4, 2015, 9:56 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/SchedulerModule.java, line 83
> > <https://reviews.apache.org/r/33689/diff/1/?file=947577#file947577line83>
> >
> >     Suggest wrapping into a PrivateModule to avoid binding annotations. The 
> > `provideTaskLaunchers` approach will go away once we deprecate/refactor 
> > TaskLauncher concept.

No longer needed now that we avoid the executor entirely.


> On May 4, 2015, 9:56 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/SchedulerModule.java, line 84
> > <https://reviews.apache.org/r/33689/diff/1/?file=947577#file947577line84>
> >
> >     This should be `AsyncUtil.loggingExecutor(1,1...)` to make sure there 
> > is always at most one thread regardless of the queue size. You can also 
> > expose the `BlockingQueue` size stat easily this way.

No longer needed now that we avoid the executor entirely.


> On May 4, 2015, 9:56 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java, line 70
> > <https://reviews.apache.org/r/33689/diff/1/?file=947578#file947578line70>
> >
> >     Expose as Arg<Interger> via SchedulerModule.

Done.


> On May 4, 2015, 9:56 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java, line 141
> > <https://reviews.apache.org/r/33689/diff/1/?file=947578#file947578line141>
> >
> >     This should be Arg<Amount<Long, Time>> exposed via SchedulerModule.

Done.


- Ben


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


On May 1, 2015, 10:55 p.m., Ben Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33689/
> -----------------------------------------------------------
> 
> (Updated May 1, 2015, 10:55 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1228
>     https://issues.apache.org/jira/browse/AURORA-1228
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Now the processing of status updates is done asynchronously with batching to 
> insulate throughput from the expensive storage resource. Updates are placed 
> into a queue and consumed by another thread. If many updates arrive while 
> we're storing a batch of updates, these will be processed together in batch 
> rather than individually.
> 
> 
> Diffs
> -----
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 
> 45de15a57baf7a2f7d437b590935714e28777f35 
>   src/main/java/org/apache/aurora/scheduler/SchedulerModule.java 
> d3ac176e9402a33fd2074b0737313458120da9e2 
>   src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
> c54619f7cd617b48069363173dcf63b6254b4095 
>   src/main/java/org/apache/aurora/scheduler/async/GcExecutorLauncher.java 
> 4d589a33a2933b0cb6caf85abfae45c5e635c3ce 
>   src/main/java/org/apache/aurora/scheduler/mesos/Driver.java 
> c7e45a89ceaa2c310feb610091eec0b04187860e 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java 
> 9b8ab7c1027731f9d3f6cae77b85272ea63354d4 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java 
> da2d5df2e053e6e1b8fb08d6813dff9eac9777f8 
>   src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 
> 32432322753799562d671db39c0d7fa308d962ff 
>   src/test/java/org/apache/aurora/scheduler/async/GcExecutorLauncherTest.java 
> 422d5a9a42310979752eb7282658316c2b772419 
>   src/test/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImplTest.java 
> abdeee49858fc439c27911c4eb544bf8e8c931d4 
> 
> Diff: https://reviews.apache.org/r/33689/diff/
> 
> 
> Testing
> -------
> 
> Ran the benchmark to confirm that this improves status update throughput 
> substantially:
> 
> Before: Around 100 updates per second for a 5ms storage latency. Much worse 
> for higher latencies.
> After:  Around 4k-5k updates per second for a 5ms storage latency, down to 3k 
> updates per second for 100ms storage latency.
> 
> Updated unit tests for the new invariants:
> 
> * TaskLaunchers are responsible for acknowledging updates.
> * UserTaskLauncher processes updates asynchronously.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Reply via email to