----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30225/#review71879 -----------------------------------------------------------
I like the shape of this, thanks for working through a few different paths. src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java <https://reviews.apache.org/r/30225/#comment117774> s/query/fetch/ src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java <https://reviews.apache.org/r/30225/#comment117776> Please document this map more thoroughly. How are entries added here, how are they removed, etc. For example, one useful detail is that we retain an entry for an update that is paused. src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java <https://reviews.apache.org/r/30225/#comment117801> Please include in test coverage some validation of the contents of this map so we can gain confidence that we do not have a memory leak. src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java <https://reviews.apache.org/r/30225/#comment117779> `storeProvider` is not used, do you need the `storage.read` at all? src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java <https://reviews.apache.org/r/30225/#comment117778> There's a race on access to `pulseStates`. Terminating an update between :282 and :287 would cause a stale entry in the map (`storage.read` doesn't offer a lock, but we shouldn't count on that anyway). Consider `synchronized (pulseStates)`. IMHO, though, this points out that this class is difficult to reason about now that multiple disparate locks are in play. Did you consider extracting an inner class to use? This would be different from the fully-isolated class we discussed before, where you can share implementation details and accept high coupling, but at least isolate management of `pulseStates` and manage it with higher-level operations from the rest of the class. Seems like you've already done this to a degree by keeping methods managing this map near each other. Your call on this. src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java <https://reviews.apache.org/r/30225/#comment117821> Coverage report indicates this line is not covered. Can you try to cover it? src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java <https://reviews.apache.org/r/30225/#comment117780> Shouldn't this be in an `else`? Looks like we've just wiped a pulse record for terminal updates, but we add another here. src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java <https://reviews.apache.org/r/30225/#comment117786> This only has one caller, consider inlining. src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java <https://reviews.apache.org/r/30225/#comment117819> Coverage report indicates an uncovered branch here. Can you try to cover it? src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java <https://reviews.apache.org/r/30225/#comment117775> At risk of being overly-verbose, how about `initializePulseMonitor`? A quick skim of `initializePulse` makes it sound like it might be simulating an inital pulse. src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java <https://reviews.apache.org/r/30225/#comment117781> Your call, but you could massage out the `clock` if you store `expirationMs` and have `getExpirationMs()` instead of storing `lastPulseMs`. src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java <https://reviews.apache.org/r/30225/#comment117791> This is never used as a `Function`, strongly consider converting to a method. Ditto elsewhere if it applies. src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java <https://reviews.apache.org/r/30225/#comment117803> I think you've done the right thing, but note that this unit test is inconsistent with your reply on review https://reviews.apache.org/r/30804/ - Bill Farner On Feb. 7, 2015, 2:43 a.m., Maxim Khutornenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30225/ > ----------------------------------------------------------- > > (Updated Feb. 7, 2015, 2:43 a.m.) > > > Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. > > > Bugs: AURORA-1010 > https://issues.apache.org/jira/browse/AURORA-1010 > > > Repository: aurora > > > Description > ------- > > Added pulsing support into the JobUpdateController. The qualified coordinated > updates get blocked until a pulse arrives. An update then becomes active and > proceeds until `blockIfNoPulsesAfterMs` expires or the update reaches a > terminal state (whichever comes first). > > UPDATE: > - Added explicit states to capture "blocked" updates > - Refactored pulseUpdate() to not rely on DB state > - Dropped JobUpdatePulseState.PAUSED as it does not seem necessary. > > > Diffs > ----- > > api/src/main/thrift/org/apache/aurora/gen/api.thrift > 4f603f9e7ed004e53937a45ea8edf7241e15f5cf > src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java > 7f2ec71d744d5fafac84291cc79feba3398d0e1e > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java > b7d8d524e15f101416889c00efc3ecf2c8d9c1a1 > src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java > d479d20259f284528b2291e2e486b36d8e47fe5e > > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java > 60f535988a20638fb16515d5027bfa65f1afb73c > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java > d3b30d48b76d8d7c64cda006a34f7ed3296526f2 > > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java > ec9d1e07abca1bf30262e1c0f741a34741e96f5d > > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java > 76460f95e058181b711fb6869f5a34c1d5bdfe31 > > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml > f9c9ceddc559b43b4a5c45c745d54ff47484edde > > src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java > ca7c0c2675477cc727ca006697665f997972dfde > > src/test/java/org/apache/aurora/scheduler/testing/FakeScheduledExecutor.java > 92cfa2b30c1c4daa3ae225fc8609fbeaecdaff7a > > src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java > 89765ac3d34a827d3748fb96a275d78e9d1b8b72 > src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java > 41d422939209d0808235128e4242c11e8ef25969 > > Diff: https://reviews.apache.org/r/30225/diff/ > > > Testing > ------- > > ./gradlew -Pq build > > > Thanks, > > Maxim Khutornenko > >
