> On Nov. 21, 2017, 9:57 a.m., Stephan Erb wrote:
> > I am not completely through but have a first set of comments.
> >
> > While reviewing this PR I got confused for second in `PruningModule.java`:
> > Please notice the difference between `HistoryPrunnerSettings` vs
> > `HistoryPrunerSettings`. Should we fix the type and refer to both settings
> > only by their full qualified name?
> >
> >
> > ```
> > 79 protected void configure() {
> > 80 // TODO(ksweeney): Create a configuration validator module so
> > this can be injected.
> > 81 // TODO(William Farner): Revert this once large task counts is
> > cheap ala hierarchichal store
> > 82 bind(HistoryPrunnerSettings.class).toInstance(new
> > HistoryPrunnerSettings(
> > 83 options.historyPruneThreshold,
> > 84 options.historyMinRetentionThreshold,
> > 85 options.historyMaxPerJobThreshold
> > 86 ));
> > 87
> > 88 bind(TaskHistoryPruner.class).in(Singleton.class);
> > 89 expose(TaskHistoryPruner.class);
> > 90 }
> > 91 });
> > 92 PubsubEventModule.bindSubscriber(binder(), TaskHistoryPruner.class);
> > 93
> > 94 install(new PrivateModule() {
> > 95 @Override
> > 96 protected void configure() {
> > 97
> > bind(JobUpdateHistoryPruner.HistoryPrunerSettings.class).toInstance(
> > 98 new JobUpdateHistoryPruner.HistoryPrunerSettings(
> > 99 options.jobUpdateHistoryPruningInterval,
> > 100 options.jobUpdateHistoryPruningThreshold,
> > 101 options.jobUpdateHistoryPerJobThreshold));
> > ```
Good call, done.
> On Nov. 21, 2017, 9:57 a.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/pruning/JobUpdateHistoryPruner.java
> > Line 103 (original), 107 (patched)
> > <https://reviews.apache.org/r/63884/diff/2/?file=1894520#file1894520line121>
> >
> > Should we track timing here? Or is this already done elsewhere?
I'm opting against until we have reason to suspect this will be expensive.
Even for O(10k) updates it should be fast.
> On Nov. 21, 2017, 9:57 a.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/pruning/JobUpdateHistoryPruner.java
> > Lines 155 (patched)
> > <https://reviews.apache.org/r/63884/diff/2/?file=1894520#file1894520line169>
> >
> > This could be large. Is it intentional to print all values here?
This matches current logging, which i assume to not be problematic.
> On Nov. 21, 2017, 9:57 a.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java
> > Lines 346-352 (patched)
> > <https://reviews.apache.org/r/63884/diff/2/?file=1894525#file1894525line346>
> >
> > I don't understand this code here. Wouldn't this also coalesce
> > completely unrelated job updates of different jobs?
You're absolutey right, this was an egregious mistake. This code will be
removed in the next iteration of the patch, but thank you for catching the
mistake!
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63884/#review191592
-----------------------------------------------------------
On Nov. 16, 2017, 10:20 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63884/
> -----------------------------------------------------------
>
> (Updated Nov. 16, 2017, 10:20 a.m.)
>
>
> Review request for Aurora, Jordan Ly and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> JobUpdateStore historically had granular APIs in the storage layer to
> minimize unnecessary use of 'expensive' database objects. The
> in-memory store makes these 'free', so moving business logic out of
> the storage layer is now feasible for performance and pragmatic.
>
> This patch also introduces idempotent log `Op`s for job updates.
> These ops are dual-read with the existing `Op`s (with the exception of
> job update pruning, which should be safe to ignore). In a future
> release (and possibly before, with a feature flag), the scheduler
> will write the idempotent `Op`s instead of the non-idempotent variants.
>
> LogStorage has always had the fundamental expectation that `Op`s are
> idempotent. The to-be-replaced `Op`s violate this requirement.
> Per `LogStorage.java`:
> This design implies that all mutations must be idempotent and free from
> constraint and thus replayable over newer operations when recovering
> from an old checkpoint.
>
>
> Diffs
> -----
>
> api/src/main/thrift/org/apache/aurora/gen/storage.thrift
> c692a5f3d715599c949c8fbef8b05c24174829e3
> src/jmh/java/org/apache/aurora/benchmark/JobUpdates.java
> a5d1894a80db13632bf03f593ead46987f1c667b
> src/jmh/java/org/apache/aurora/benchmark/UpdateStoreBenchmarks.java
> c98c514bfc28afaad44bda355720347dd9ee2d3e
>
> src/main/java/org/apache/aurora/scheduler/pruning/JobUpdateHistoryPruner.java
> b2768d98f7c7b0632cd31656640fcb053bcd3be1
> src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java
> 7f8c66cae388571cd3eef2668d1b42e3925dc7e8
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java
> b3d906b96f9bfe6e4d3fcc9f3a5d02819a1ef9fe
> src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java
> 3ce2c7fc06082ae0ca7a77a33bb7f333212a048c
>
> src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java
> 57c483b049c40b56d8c83e3dcce67de00113853b
>
> src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java
> baf2647c54f1f9918139584e5151873a3853e83c
>
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java
> 4d051fc4ab280418c7df70924c5e5b496c2ce052
>
> src/main/java/org/apache/aurora/scheduler/storage/mem/MemJobUpdateStore.java
> 826cee9c5c27b80dec55af6fb64fbe9bd72fe03e
> src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java
> bba1161a48738e19f10fcf72395f8d70b481ed13
>
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
> dc8d11c5003b2637b921b6afa6e2cb5f13102858
>
> src/test/java/org/apache/aurora/scheduler/pruning/JobUpdateHistoryPrunerTest.java
> 74db5ecd8f63a2df4e24ca83a03c22e90936bbae
> src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java
> 6be4a9c4145ab38905f153a07a022707186a862c
>
> src/test/java/org/apache/aurora/scheduler/storage/AbstractJobUpdateStoreTest.java
> 9fab7aeb99b155cc55c778c9b27e92daa97fe47e
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java
> c208210497454fabd7b3c6d41b4b5f40b1a6bb69
>
> src/test/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorageTest.java
> d5e5c11ad09415e861edfaa521c53fd7904d1bfd
>
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
> 39456cfe8b8c8e198a6f8c82e09769bd75db2adc
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java
> 661ce58a840e5a23ff5438517880877cd90d0537
>
>
> Diff: https://reviews.apache.org/r/63884/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
