-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63884/#review191592
-----------------------------------------------------------
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));
```
src/main/java/org/apache/aurora/scheduler/pruning/JobUpdateHistoryPruner.java
Line 103 (original), 107 (patched)
<https://reviews.apache.org/r/63884/#comment269438>
Should we track timing here? Or is this already done elsewhere?
src/main/java/org/apache/aurora/scheduler/pruning/JobUpdateHistoryPruner.java
Lines 155 (patched)
<https://reviews.apache.org/r/63884/#comment269439>
This could be large. Is it intentional to print all values here?
src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java
Lines 346-352 (patched)
<https://reviews.apache.org/r/63884/#comment269445>
I don't understand this code here. Wouldn't this also coalesce completely
unrelated job updates of different jobs?
- Stephan Erb
On Nov. 16, 2017, 7:20 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63884/
> -----------------------------------------------------------
>
> (Updated Nov. 16, 2017, 7:20 p.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
>
>
