You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Bill Farner <wf...@apache.org> on 2017/11/16 18:20:11 UTC
Review Request 63884: Idempotent Ops and slimmed API for
JobUpdateStore
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63884/
-----------------------------------------------------------
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
Re: Review Request 63884: Idempotent Ops and slimmed API for
JobUpdateStore
Posted by Bill Farner <wf...@apache.org>.
> On Nov. 21, 2017, 10:53 a.m., David McLaughlin wrote:
> > Are we sure this was purely a database optimization? This looks like it will now write the entire JobUpdateDetails to the replicated log for every instance event change? In large clusters this could impact write log write latency (and size) compared to just writing events. It would be nice to run a scale test to validate.
> This looks like it will now write the entire JobUpdateDetails to the replicated log for every instance event change
That's correct. This is the least complex way to achieve idempotency for update events. I have not yet weighed the effects of this on persistent storage, but it could indeed be prohibitive for extremely large jobs.
`RemoveJobUpdates` is the more critical feature for this patch anyhow, so i will take a different approach for saving update events and spare any concern about storage impact.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63884/#review191621
-----------------------------------------------------------
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
>
>
Re: Review Request 63884: Idempotent Ops and slimmed API for
JobUpdateStore
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63884/#review191621
-----------------------------------------------------------
Are we sure this was purely a database optimization? This looks like it will now write the entire JobUpdateDetails to the replicated log for every instance event change? In large clusters this could impact write log write latency (and size) compared to just writing events. It would be nice to run a scale test to validate.
- David McLaughlin
On Nov. 16, 2017, 6: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, 6: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
>
>
Re: Review Request 63884: Idempotent Ops and slimmed API for
JobUpdateStore
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63884/#review191237
-----------------------------------------------------------
Ship it!
Master (46b1112) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Nov. 16, 2017, 6: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, 6: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
>
>
Re: Review Request 63884: Add RemoveJobUpdates log Op,
slim JobUpdateStore API
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63884/#review192073
-----------------------------------------------------------
Ship it!
Master (80139da) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Nov. 21, 2017, 11:44 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63884/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2017, 11:44 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 the `RemoveJobUpdates` log `Op`, and
> `PruneJobUpdateHistory` is now ignored. In a future release (and possibly
> before, with a feature flag), the scheduler will write `RemoveJobUpdates`
> to the log.
>
> LogStorage has always had the fundamental expectation that `Op`s are
> idempotent. The job update event `Op`s arguably violate this requirement, but
> at minimum, explicit removal of updates is necessary for idempotency.
>
> From 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/pruning/PruningModule.java 4433b962fe73bafc0b2e06edcb9f7b117c403715
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 3cafbc2df4361458cbbcc3c1f345706069ba46d1
> 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/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/pruning/TaskHistoryPrunerTest.java 5e5c51814c12a51abada9642b550e06533f73094
> 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/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 63884: Add RemoveJobUpdates log Op,
slim JobUpdateStore API
Posted by Bill Farner <wf...@apache.org>.
> On Nov. 21, 2017, 4:27 p.m., David McLaughlin wrote:
> > src/main/java/org/apache/aurora/scheduler/pruning/JobUpdateHistoryPruner.java
> > Line 103 (original), 107 (patched)
> > <https://reviews.apache.org/r/63884/diff/3/?file=1899269#file1899269line121>
> >
> > Can we track timings for this method? I believe we had that for the old pruner.
Done, as you are the second reviewer to request this :-)
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63884/#review191683
-----------------------------------------------------------
On Nov. 21, 2017, 3:44 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63884/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2017, 3:44 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 the `RemoveJobUpdates` log `Op`, and
> `PruneJobUpdateHistory` is now ignored. In a future release (and possibly
> before, with a feature flag), the scheduler will write `RemoveJobUpdates`
> to the log.
>
> LogStorage has always had the fundamental expectation that `Op`s are
> idempotent. The job update event `Op`s arguably violate this requirement, but
> at minimum, explicit removal of updates is necessary for idempotency.
>
> From 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/pruning/PruningModule.java 4433b962fe73bafc0b2e06edcb9f7b117c403715
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 3cafbc2df4361458cbbcc3c1f345706069ba46d1
> 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/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/pruning/TaskHistoryPrunerTest.java 5e5c51814c12a51abada9642b550e06533f73094
> 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/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 63884: Add RemoveJobUpdates log Op,
slim JobUpdateStore API
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63884/#review191683
-----------------------------------------------------------
This LGTM.
src/main/java/org/apache/aurora/scheduler/pruning/JobUpdateHistoryPruner.java
Line 103 (original), 107 (patched)
<https://reviews.apache.org/r/63884/#comment269537>
Can we track timings for this method? I believe we had that for the old pruner.
- David McLaughlin
On Nov. 21, 2017, 11:44 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63884/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2017, 11:44 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 the `RemoveJobUpdates` log `Op`, and
> `PruneJobUpdateHistory` is now ignored. In a future release (and possibly
> before, with a feature flag), the scheduler will write `RemoveJobUpdates`
> to the log.
>
> LogStorage has always had the fundamental expectation that `Op`s are
> idempotent. The job update event `Op`s arguably violate this requirement, but
> at minimum, explicit removal of updates is necessary for idempotency.
>
> From 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/pruning/PruningModule.java 4433b962fe73bafc0b2e06edcb9f7b117c403715
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 3cafbc2df4361458cbbcc3c1f345706069ba46d1
> 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/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/pruning/TaskHistoryPrunerTest.java 5e5c51814c12a51abada9642b550e06533f73094
> 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/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 63884: Add RemoveJobUpdates log Op,
slim JobUpdateStore API
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63884/#review192045
-----------------------------------------------------------
Ship it!
Ship It!
- David McLaughlin
On Nov. 21, 2017, 11:44 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63884/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2017, 11:44 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 the `RemoveJobUpdates` log `Op`, and
> `PruneJobUpdateHistory` is now ignored. In a future release (and possibly
> before, with a feature flag), the scheduler will write `RemoveJobUpdates`
> to the log.
>
> LogStorage has always had the fundamental expectation that `Op`s are
> idempotent. The job update event `Op`s arguably violate this requirement, but
> at minimum, explicit removal of updates is necessary for idempotency.
>
> From 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/pruning/PruningModule.java 4433b962fe73bafc0b2e06edcb9f7b117c403715
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 3cafbc2df4361458cbbcc3c1f345706069ba46d1
> 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/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/pruning/TaskHistoryPrunerTest.java 5e5c51814c12a51abada9642b550e06533f73094
> 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/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 63884: Add RemoveJobUpdates log Op,
slim JobUpdateStore API
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63884/#review191679
-----------------------------------------------------------
Ship it!
Master (c746452) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Nov. 21, 2017, 11:44 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63884/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2017, 11:44 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 the `RemoveJobUpdates` log `Op`, and
> `PruneJobUpdateHistory` is now ignored. In a future release (and possibly
> before, with a feature flag), the scheduler will write `RemoveJobUpdates`
> to the log.
>
> LogStorage has always had the fundamental expectation that `Op`s are
> idempotent. The job update event `Op`s arguably violate this requirement, but
> at minimum, explicit removal of updates is necessary for idempotency.
>
> From 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/pruning/PruningModule.java 4433b962fe73bafc0b2e06edcb9f7b117c403715
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 3cafbc2df4361458cbbcc3c1f345706069ba46d1
> 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/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/pruning/TaskHistoryPrunerTest.java 5e5c51814c12a51abada9642b550e06533f73094
> 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/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 63884: Add RemoveJobUpdates log Op,
slim JobUpdateStore API
Posted by Stephan Erb <se...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63884/#review192075
-----------------------------------------------------------
Ship it!
Ship It!
- Stephan Erb
On Nov. 22, 2017, 12:44 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63884/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2017, 12:44 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 the `RemoveJobUpdates` log `Op`, and
> `PruneJobUpdateHistory` is now ignored. In a future release (and possibly
> before, with a feature flag), the scheduler will write `RemoveJobUpdates`
> to the log.
>
> LogStorage has always had the fundamental expectation that `Op`s are
> idempotent. The job update event `Op`s arguably violate this requirement, but
> at minimum, explicit removal of updates is necessary for idempotency.
>
> From 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/pruning/PruningModule.java 4433b962fe73bafc0b2e06edcb9f7b117c403715
> src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 3cafbc2df4361458cbbcc3c1f345706069ba46d1
> 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/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/pruning/TaskHistoryPrunerTest.java 5e5c51814c12a51abada9642b550e06533f73094
> 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/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 63884: Add RemoveJobUpdates log Op,
slim JobUpdateStore API
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63884/
-----------------------------------------------------------
(Updated Nov. 21, 2017, 3:44 p.m.)
Review request for Aurora, Jordan Ly and Stephan Erb.
Changes
-------
Addressed review feedback.
Summary (updated)
-----------------
Add RemoveJobUpdates log Op, slim JobUpdateStore API
Repository: aurora
Description (updated)
-------
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 the `RemoveJobUpdates` log `Op`, and
`PruneJobUpdateHistory` is now ignored. In a future release (and possibly
before, with a feature flag), the scheduler will write `RemoveJobUpdates`
to the log.
LogStorage has always had the fundamental expectation that `Op`s are
idempotent. The job update event `Op`s arguably violate this requirement, but
at minimum, explicit removal of updates is necessary for idempotency.
From 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 (updated)
-----
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/pruning/PruningModule.java 4433b962fe73bafc0b2e06edcb9f7b117c403715
src/main/java/org/apache/aurora/scheduler/pruning/TaskHistoryPruner.java 3cafbc2df4361458cbbcc3c1f345706069ba46d1
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/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/pruning/TaskHistoryPrunerTest.java 5e5c51814c12a51abada9642b550e06533f73094
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/3/
Changes: https://reviews.apache.org/r/63884/diff/2-3/
Testing
-------
Thanks,
Bill Farner
Re: Review Request 63884: Idempotent Ops and slimmed API for
JobUpdateStore
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63884/#review191580
-----------------------------------------------------------
Ping
- 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/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 63884: Idempotent Ops and slimmed API for
JobUpdateStore
Posted by Bill Farner <wf...@apache.org>.
> 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
>
>
Re: Review Request 63884: Idempotent Ops and slimmed API for
JobUpdateStore
Posted by Stephan Erb <se...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 63884: Idempotent Ops and slimmed API for
JobUpdateStore
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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
>
>