----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63884/#review191206 -----------------------------------------------------------
Annotated the diff with some reviewer notes. src/jmh/java/org/apache/aurora/benchmark/JobUpdates.java Line 70 (original), 68 (patched) <https://reviews.apache.org/r/63884/#comment268857> Events are now saved here, so the subsequent saves are unnecessary. src/main/java/org/apache/aurora/scheduler/pruning/JobUpdateHistoryPruner.java Line 42 (original), 54 (patched) <https://reviews.apache.org/r/63884/#comment268858> This is now an `AbstractScheduledService`, so the injected executor is unneeded. Additionally, pruning logic was moved here. src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java Lines 357 (patched) <https://reviews.apache.org/r/63884/#comment268860> Reads the new Op type which will be written in the future. src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java Line 369 (original), 396 (patched) <https://reviews.apache.org/r/63884/#comment268861> Copying comment from `WriteAheadStorage.java` for rationale: > Pruned updates will eventually go away from persisted storage when a new snapshot is cut. > So, persisting pruning attempts is not strictly necessary as the periodic pruner will > provide eventual consistency between volatile and persistent storage upon scheduler > restart. By generating an out of band pruning during log replay the consistency is > achieved sooner without potentially exposing pruned but not yet persisted data. With this behavior in mind, the `PRUNE_JOB_UPDATE_HISTORY` op is no longer read nor written. src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java Lines 299 (patched) <https://reviews.apache.org/r/63884/#comment268863> This will be useful in the future to reduce log write activity for `SAVE_JOB_UPDATE` `Op`s. src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java Lines 218-223 (original), 213-223 (patched) <https://reviews.apache.org/r/63884/#comment268865> This generates a large number of `Op`s for very large updates, but the overall space cost should be roughly equivalent to the future mode where the `JobUpdateDetails` is written directly. Reminder - `write()` appends to the in-memory transaction, which is persisted as a whole when the transaction completes. src/main/java/org/apache/aurora/scheduler/storage/mem/MemJobUpdateStore.java Lines 76-77 (original) <https://reviews.apache.org/r/63884/#comment268866> These stats were moved to `JobUpdateControllerImpl`. src/test/java/org/apache/aurora/scheduler/storage/AbstractJobUpdateStoreTest.java Lines 294-301 (original) <https://reviews.apache.org/r/63884/#comment268868> Removing this test is necessary for idempotency. We must be able to save the same update twice to no effect, and we must be able to save a new version of an update to overwrite the original. The uniqueness constraint is enforced at the non-storage caller, `JobUpdateControllerImpl`. src/test/java/org/apache/aurora/scheduler/storage/AbstractJobUpdateStoreTest.java Lines 451 (patched) <https://reviews.apache.org/r/63884/#comment268869> Whoops, i need to finish this. Coming in an updated draft. - Bill Farner 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/1/ > > > Testing > ------- > > > Thanks, > > Bill Farner > >
