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 2014/08/15 03:11:30 UTC
Review Request 24727: Store a lock association with job updates.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24727/
-----------------------------------------------------------
Review request for Aurora and Maxim Khutornenko.
Repository: aurora
Description
-------
Store a lock association with job updates.
Diffs
-----
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/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/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
Diff: https://reviews.apache.org/r/24727/diff/
Testing
-------
./gradlew build -Pq
Thanks,
Bill Farner
Re: Review Request 24727: Store a lock association with job updates.
Posted by Bill Farner <wf...@apache.org>.
> On Aug. 15, 2014, 1:54 a.m., Maxim Khutornenko wrote:
> > src/main/thrift/org/apache/aurora/gen/storage.thrift, line 77
> > <https://reviews.apache.org/r/24727/diff/1/?file=661129#file661129line77>
> >
> > The SaveJobUpdate could also benefit from this comment.
Done.
> On Aug. 15, 2014, 1:54 a.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml, line 329
> > <https://reviews.apache.org/r/24727/diff/1/?file=661127#file661127line329>
> >
> > This data is only needed when saving snapshots (fetchAllJobUpdateDetails). Should we rather move this join into selectAllDetails and keep selectDetails as is for the perf reasons?
> >
> > It would require splitting this sql block into a few refs though, so up to you.
I'll take the reduced complexity over the performance for now. If this join poses a performance problem, we've got bigger issues here :-)
> On Aug. 15, 2014, 1:54 a.m., Maxim Khutornenko wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java, line 372
> > <https://reviews.apache.org/r/24727/diff/1/?file=661132#file661132line372>
> >
> > Should be easier to read with '\n' after every test case here.
Good call, done.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24727/#review50691
-----------------------------------------------------------
On Aug. 15, 2014, 1:11 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, 1:11 a.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Store a lock association with job updates.
>
>
> Diffs
> -----
>
> 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/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/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
>
> Diff: https://reviews.apache.org/r/24727/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 24727: Store a lock association with job updates.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24727/#review50691
-----------------------------------------------------------
Ship it!
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml
<https://reviews.apache.org/r/24727/#comment88557>
This data is only needed when saving snapshots (fetchAllJobUpdateDetails). Should we rather move this join into selectAllDetails and keep selectDetails as is for the perf reasons?
It would require splitting this sql block into a few refs though, so up to you.
src/main/thrift/org/apache/aurora/gen/storage.thrift
<https://reviews.apache.org/r/24727/#comment88559>
The SaveJobUpdate could also benefit from this comment.
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
<https://reviews.apache.org/r/24727/#comment88560>
Should be easier to read with '\n' after every test case here.
- Maxim Khutornenko
On Aug. 15, 2014, 1:11 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, 1:11 a.m.)
>
>
> Review request for Aurora and Maxim Khutornenko.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Store a lock association with job updates.
>
>
> Diffs
> -----
>
> 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/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/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
>
> Diff: https://reviews.apache.org/r/24727/diff/
>
>
> Testing
> -------
>
> ./gradlew build -Pq
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 24727: Store a lock association with job updates.
Posted by Bill Farner <wf...@apache.org>.
> 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.
> 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.
- Bill
-----------------------------------------------------------
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
>
>
Re: Review Request 24727: Store a lock association with job updates.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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.
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).
- 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
>
>
Re: Review Request 24727: Store a lock association with job updates.
Posted by Bill Farner <wf...@apache.org>.
> 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
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.
- Bill
-----------------------------------------------------------
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
>
>
Re: Review Request 24727: Store a lock association with job updates.
Posted by Bill Farner <wf...@apache.org>.
> 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.
>
> Maxim Khutornenko wrote:
> Well, the only problem is that feature already exists :) Shutting it down will required more efforts than keeping it.
As discussed offline, the goal is not to remove lock-related RPCs. Let's continue with the implicit locking in updates, as doing otherwise requires the user to also release the lock at an unknown point in the future.
- Bill
-----------------------------------------------------------
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
>
>
Re: Review Request 24727: Store a lock association with job updates.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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
>
>
Re: Review Request 24727: Store a lock association with job updates.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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?
s/hense/hence
- 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
>
>
Re: Review Request 24727: Store a lock association with job updates.
Posted by Bill Farner <wf...@apache.org>.
> 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).
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.
- Bill
-----------------------------------------------------------
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
>
>
Re: Review Request 24727: Store a lock association with job updates.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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.
| 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.
- 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
>
>
Re: Review Request 24727: Store a lock association with job updates.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24727/#review50723
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
<https://reviews.apache.org/r/24727/#comment88585>
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
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
>
>
Re: Review Request 24727: Store a lock association with job updates.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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
Re: Review Request 24727: Store a lock association with job updates.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24727/
-----------------------------------------------------------
(Updated Aug. 15, 2014, 3:16 a.m.)
Review request for Aurora and Maxim Khutornenko.
Changes
-------
Addressed comments and fixed some compilation errors. Not sure exactly how, but my desktop had a false green build, noticed the errors on my laptop.
Repository: aurora
Description
-------
Store a lock association with job updates.
Diffs (updated)
-----
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