> On Aug. 15, 2014, 3:47 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, > > lines 1293-1299 > > <https://reviews.apache.org/r/24727/diff/2/?file=661215#file661215line1293> > > > > Why this change? The lock is supposed to be already acquired by the > > time startJobUpdate is executed, hense the Lock instance in the argument > > set. How is it supposed to work now? > > Maxim Khutornenko wrote: > s/hense/hence > > Bill Farner wrote: > Sounds like we had different visions for how this would work. I'm > imagining that only a single RPC is required to start an update, and > `startJobUpdate` is it. > > Maxim Khutornenko wrote: > I was mulling over this yesterday but ultimately decided there is still > value in keeping it separate. The job lock is not the update specific concept > and may be acquired by a different action before the update is ready to get > started (i.e. by a pre-update approval hook). Having a startJobUpdate > "special" in this sense breaks the consistency pattern with other APIs and > makes acquireLock API redundant. I don't think we should specialize the > otherwise generic job lock idea as it may be useful when we finally have a > hierarchical job/task storage where locks could be applied at different > levels (role, env, job). > > Bill Farner wrote: > Sounds like that complicates the majority use case, where that behavior > is not needed. I'd be fine with adding a feature to provide an > already-secured lock. But i really dislike the idea of always requiring two > RPCs. > > Maxim Khutornenko wrote: > | I'd be fine with adding a feature to provide an already-secured lock. > > Not sure I understand, this feature is the existing acquireLock RPC, no? > > > | But i really dislike the idea of always requiring two RPCs. > > This is how it works today and it does not feel like a huge price to pay > for something that may become handy in future. > > Bill Farner wrote: > > Not sure I understand, this feature is the existing acquireLock RPC, no? > > It is. I'm saying i wouldn't mind adding a feature for startJobUpdate to > accept an optional Lock. > > > This is how it works today and it does not feel like a huge price to > pay for something that may become handy in future. > > I defer to YAGNI principle. Let's add the feature when we actually have > a use for it.
Well, the only problem is that feature already exists :) Shutting it down will required more efforts than keeping it. - Maxim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24727/#review50723 ----------------------------------------------------------- On Aug. 15, 2014, 3:19 a.m., Bill Farner wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24727/ > ----------------------------------------------------------- > > (Updated Aug. 15, 2014, 3:19 a.m.) > > > Review request for Aurora and Maxim Khutornenko. > > > Bugs: AURORA-613 > https://issues.apache.org/jira/browse/AURORA-613 > > > Repository: aurora > > > Description > ------- > > Store a lock association with job updates. > > > Diffs > ----- > > src/main/java/org/apache/aurora/scheduler/state/JobUpdater.java > f15344417f52fe5e909abfbba636c48277404fb4 > src/main/java/org/apache/aurora/scheduler/state/JobUpdaterImpl.java > 6bcdf620c993091999c6cccaeae023cb061cbd50 > src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java > 3f083d60a882c6665d9891172edb9a5aeddade9b > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java > c05833f30eaf79527599a7223791a6f4f5309388 > src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java > d659aa124bd702589952da1b19854a00862b0c86 > > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java > d590219e9ff873bc7b9b740759b024c463c523cf > src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java > 342bab0426ebeeab0b3d2d038d98dba1de836231 > > src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java > 3d291dd21892c20a8eb9388744c8b2f10a811554 > > src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java > 2915ff00f3a2e3602414cedebbb0270e07dc869a > > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java > 0802ee08601f5b9747f99823679d92af59a76cbc > > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml > 17c58b1a07f2fcef7fe8502540b347542b603e60 > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql > 1cf803fe019290c042e5a73824e39a12072b2431 > src/main/thrift/org/apache/aurora/gen/storage.thrift > 9f8378ea3f5d386c3177207296bf4b436b730b45 > src/main/thrift/org/apache/aurora/gen/storage_local.thrift > becfd7528610a32af907489d021319d9b371c332 > src/test/java/org/apache/aurora/scheduler/state/JobUpdaterImplTest.java > 1f985fb75cff7e120bce9e60b91a19c7e19b9c2a > src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java > 2f2c3e12657e9af1edf819ff95d0da5db0c5de4b > > src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java > f695b85514bcc5cedb16e962124af3db052cb17a > src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java > ae4cef42d67556a22ea45eaa6d7542915924ffd1 > src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java > ebcb9103d75909080f5b6a69db3a1bf46cfd9780 > > src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplTest.java > bee9c9c1fb43c5703c291edc51cb1bb73aefc8e5 > > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java > 7dbf97a6cecef928f563e76d37488816ca91872a > > Diff: https://reviews.apache.org/r/24727/diff/ > > > Testing > ------- > > ./gradlew build -Pq > > > Thanks, > > Bill Farner > >