Re: Review Request 24744: Dropping lock from startJobUpdate parameters.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24744/#review50900 --- Ship it! Ship It! - Bill Farner On Aug. 15, 2014, 11:57 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24744/ --- (Updated Aug. 15, 2014, 11:57 p.m.) Review request for Aurora, Mark Chu-Carroll and Bill Farner. Repository: aurora Description --- The job lock is now acquired by the startJobUpdate call. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 7ef28858ad290c74248b89c49d2a684eb1c7127e src/main/python/apache/aurora/client/api/__init__.py 62de93bb942adab47590112b76c365fac7877371 src/main/thrift/org/apache/aurora/gen/api.thrift af9f02ed1de487bc5cc2967d2edcece5b21e0be5 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 649afa24b2cfc9a1d67d350473e439d209bd720c src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 38bc9ed1ea17cac00920a2f1d066458badd7b4bb src/test/python/apache/aurora/client/api/test_api.py 96db25d9f7492a0d49e98af27f17b6cee19f5a49 src/test/python/apache/aurora/client/api/test_scheduler_client.py ab74db34d61e72c50d4ac9252b02cbf69377d194 src/test/resources/org/apache/aurora/gen/api.thrift.md5 21a05f6939da1dd7fc15cf6336bc3fee283f16ab src/test/resources/org/apache/aurora/gen/storage.thrift.md5 45762990d33969bedde7340887cde16a535e99fe Diff: https://reviews.apache.org/r/24744/diff/ Testing --- gradle -Pq build ./pants src/test/python/apache/aurora/client/api:: Thanks, Maxim Khutornenko
Re: Review Request 24744: Dropping lock from startJobUpdate parameters.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24744/#review50930 --- Ship it! Looks fine, but I don't really understand the reasoning. To me, it seems like expecting someone to have a lockid for a pause or a resume call is a good idea. It allows someone to pass responsibility for an ongoing update to another person, without having to allow anyone at all to interrupt/resume it. - Mark Chu-Carroll On Aug. 18, 2014, 4:10 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24744/ --- (Updated Aug. 18, 2014, 4:10 p.m.) Review request for Aurora, Mark Chu-Carroll and Bill Farner. Repository: aurora Description --- The job lock is now acquired by the startJobUpdate call. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 7ef28858ad290c74248b89c49d2a684eb1c7127e src/main/python/apache/aurora/client/api/__init__.py 62de93bb942adab47590112b76c365fac7877371 src/main/thrift/org/apache/aurora/gen/api.thrift 2b376d663b3bc9264965db58ef89de59b10169ad src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 649afa24b2cfc9a1d67d350473e439d209bd720c src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 38bc9ed1ea17cac00920a2f1d066458badd7b4bb src/test/python/apache/aurora/client/api/test_api.py 96db25d9f7492a0d49e98af27f17b6cee19f5a49 src/test/python/apache/aurora/client/api/test_scheduler_client.py ab74db34d61e72c50d4ac9252b02cbf69377d194 src/test/resources/org/apache/aurora/gen/api.thrift.md5 0be4d96782769a2097ce10f8872c4300e9f74e5a Diff: https://reviews.apache.org/r/24744/diff/ Testing --- gradle -Pq build ./pants src/test/python/apache/aurora/client/api:: Thanks, Maxim Khutornenko
Re: Review Request 24744: Dropping lock from startJobUpdate parameters.
On Aug. 18, 2014, 8:16 p.m., Mark Chu-Carroll wrote: Looks fine, but I don't really understand the reasoning. To me, it seems like expecting someone to have a lockid for a pause or a resume call is a good idea. It allows someone to pass responsibility for an ongoing update to another person, without having to allow anyone at all to interrupt/resume it. With the async updater sharing a lock among users or even accross same user sessions becomes a problem. It should be easier to allow update mutations to any authorized user, which is true even today (e.g. cancel_update). - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24744/#review50930 --- On Aug. 18, 2014, 8:10 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24744/ --- (Updated Aug. 18, 2014, 8:10 p.m.) Review request for Aurora, Mark Chu-Carroll and Bill Farner. Repository: aurora Description --- The job lock is now acquired by the startJobUpdate call. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 7ef28858ad290c74248b89c49d2a684eb1c7127e src/main/python/apache/aurora/client/api/__init__.py 62de93bb942adab47590112b76c365fac7877371 src/main/thrift/org/apache/aurora/gen/api.thrift 2b376d663b3bc9264965db58ef89de59b10169ad src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 649afa24b2cfc9a1d67d350473e439d209bd720c src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 38bc9ed1ea17cac00920a2f1d066458badd7b4bb src/test/python/apache/aurora/client/api/test_api.py 96db25d9f7492a0d49e98af27f17b6cee19f5a49 src/test/python/apache/aurora/client/api/test_scheduler_client.py ab74db34d61e72c50d4ac9252b02cbf69377d194 src/test/resources/org/apache/aurora/gen/api.thrift.md5 0be4d96782769a2097ce10f8872c4300e9f74e5a Diff: https://reviews.apache.org/r/24744/diff/ Testing --- gradle -Pq build ./pants src/test/python/apache/aurora/client/api:: Thanks, Maxim Khutornenko
Review Request 24744: Dropping lock from startJobUpdate parameters.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24744/ --- Review request for Aurora, Mark Chu-Carroll and Bill Farner. Repository: aurora Description --- The job lock is now acquired by the startJobUpdate call. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 7ef28858ad290c74248b89c49d2a684eb1c7127e src/main/python/apache/aurora/client/api/__init__.py 62de93bb942adab47590112b76c365fac7877371 src/main/thrift/org/apache/aurora/gen/api.thrift af9f02ed1de487bc5cc2967d2edcece5b21e0be5 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 649afa24b2cfc9a1d67d350473e439d209bd720c src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 38bc9ed1ea17cac00920a2f1d066458badd7b4bb src/test/python/apache/aurora/client/api/test_api.py 96db25d9f7492a0d49e98af27f17b6cee19f5a49 src/test/python/apache/aurora/client/api/test_scheduler_client.py ab74db34d61e72c50d4ac9252b02cbf69377d194 src/test/resources/org/apache/aurora/gen/api.thrift.md5 21a05f6939da1dd7fc15cf6336bc3fee283f16ab src/test/resources/org/apache/aurora/gen/storage.thrift.md5 45762990d33969bedde7340887cde16a535e99fe Diff: https://reviews.apache.org/r/24744/diff/ Testing --- gradle -Pq build ./pants src/test/python/apache/aurora/client/api:: Thanks, Maxim Khutornenko
Re: Review Request 24744: Dropping lock from startJobUpdate parameters.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24744/#review50755 --- src/main/thrift/org/apache/aurora/gen/api.thrift https://reviews.apache.org/r/24744/#comment88618 We really should not expose the lock. Any attempt to do anything with the lock will ~certainly interfere with the updater. - Bill Farner On Aug. 15, 2014, 6:14 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24744/ --- (Updated Aug. 15, 2014, 6:14 p.m.) Review request for Aurora, Mark Chu-Carroll and Bill Farner. Repository: aurora Description --- The job lock is now acquired by the startJobUpdate call. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 7ef28858ad290c74248b89c49d2a684eb1c7127e src/main/python/apache/aurora/client/api/__init__.py 62de93bb942adab47590112b76c365fac7877371 src/main/thrift/org/apache/aurora/gen/api.thrift af9f02ed1de487bc5cc2967d2edcece5b21e0be5 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 649afa24b2cfc9a1d67d350473e439d209bd720c src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 38bc9ed1ea17cac00920a2f1d066458badd7b4bb src/test/python/apache/aurora/client/api/test_api.py 96db25d9f7492a0d49e98af27f17b6cee19f5a49 src/test/python/apache/aurora/client/api/test_scheduler_client.py ab74db34d61e72c50d4ac9252b02cbf69377d194 src/test/resources/org/apache/aurora/gen/api.thrift.md5 21a05f6939da1dd7fc15cf6336bc3fee283f16ab src/test/resources/org/apache/aurora/gen/storage.thrift.md5 45762990d33969bedde7340887cde16a535e99fe Diff: https://reviews.apache.org/r/24744/diff/ Testing --- gradle -Pq build ./pants src/test/python/apache/aurora/client/api:: Thanks, Maxim Khutornenko
Re: Review Request 24744: Dropping lock from startJobUpdate parameters.
On Aug. 15, 2014, 6:20 p.m., Bill Farner wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 751 https://reviews.apache.org/r/24744/diff/1/?file=661623#file661623line751 We really should not expose the lock. Any attempt to do anything with the lock will ~certainly interfere with the updater. How do we ensure pause/resume/abort is authorized to act on the update then? Sure, storing the lock on the client is not a good idea but unless we have some secondary way to authorize the action anyone could interfere with the update given its ID. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24744/#review50755 --- On Aug. 15, 2014, 6:14 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24744/ --- (Updated Aug. 15, 2014, 6:14 p.m.) Review request for Aurora, Mark Chu-Carroll and Bill Farner. Repository: aurora Description --- The job lock is now acquired by the startJobUpdate call. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 7ef28858ad290c74248b89c49d2a684eb1c7127e src/main/python/apache/aurora/client/api/__init__.py 62de93bb942adab47590112b76c365fac7877371 src/main/thrift/org/apache/aurora/gen/api.thrift af9f02ed1de487bc5cc2967d2edcece5b21e0be5 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 649afa24b2cfc9a1d67d350473e439d209bd720c src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 38bc9ed1ea17cac00920a2f1d066458badd7b4bb src/test/python/apache/aurora/client/api/test_api.py 96db25d9f7492a0d49e98af27f17b6cee19f5a49 src/test/python/apache/aurora/client/api/test_scheduler_client.py ab74db34d61e72c50d4ac9252b02cbf69377d194 src/test/resources/org/apache/aurora/gen/api.thrift.md5 21a05f6939da1dd7fc15cf6336bc3fee283f16ab src/test/resources/org/apache/aurora/gen/storage.thrift.md5 45762990d33969bedde7340887cde16a535e99fe Diff: https://reviews.apache.org/r/24744/diff/ Testing --- gradle -Pq build ./pants src/test/python/apache/aurora/client/api:: Thanks, Maxim Khutornenko
Re: Review Request 24744: Dropping lock from startJobUpdate parameters.
On Aug. 15, 2014, 6:20 p.m., Bill Farner wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 751 https://reviews.apache.org/r/24744/diff/1/?file=661623#file661623line751 We really should not expose the lock. Any attempt to do anything with the lock will ~certainly interfere with the updater. Maxim Khutornenko wrote: How do we ensure pause/resume/abort is authorized to act on the update then? Sure, storing the lock on the client is not a good idea but unless we have some secondary way to authorize the action anyone could interfere with the update given its ID. Bill Farner wrote: That's exactly what we need. Abort/pause/resume are unscoped, big red buttons. So, anyone authorized to act on the job would be able to interfere? I thought the big red button would be the cancel_update with the abort/pause/resume allowed only to the update owner. In that case, I need to clear all job update apis from the Lock concept. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24744/#review50755 --- On Aug. 15, 2014, 6:14 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24744/ --- (Updated Aug. 15, 2014, 6:14 p.m.) Review request for Aurora, Mark Chu-Carroll and Bill Farner. Repository: aurora Description --- The job lock is now acquired by the startJobUpdate call. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 7ef28858ad290c74248b89c49d2a684eb1c7127e src/main/python/apache/aurora/client/api/__init__.py 62de93bb942adab47590112b76c365fac7877371 src/main/thrift/org/apache/aurora/gen/api.thrift af9f02ed1de487bc5cc2967d2edcece5b21e0be5 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 649afa24b2cfc9a1d67d350473e439d209bd720c src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 38bc9ed1ea17cac00920a2f1d066458badd7b4bb src/test/python/apache/aurora/client/api/test_api.py 96db25d9f7492a0d49e98af27f17b6cee19f5a49 src/test/python/apache/aurora/client/api/test_scheduler_client.py ab74db34d61e72c50d4ac9252b02cbf69377d194 src/test/resources/org/apache/aurora/gen/api.thrift.md5 21a05f6939da1dd7fc15cf6336bc3fee283f16ab src/test/resources/org/apache/aurora/gen/storage.thrift.md5 45762990d33969bedde7340887cde16a535e99fe Diff: https://reviews.apache.org/r/24744/diff/ Testing --- gradle -Pq build ./pants src/test/python/apache/aurora/client/api:: Thanks, Maxim Khutornenko
Re: Review Request 24744: Dropping lock from startJobUpdate parameters.
On Aug. 15, 2014, 6:20 p.m., Bill Farner wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 751 https://reviews.apache.org/r/24744/diff/1/?file=661623#file661623line751 We really should not expose the lock. Any attempt to do anything with the lock will ~certainly interfere with the updater. Maxim Khutornenko wrote: How do we ensure pause/resume/abort is authorized to act on the update then? Sure, storing the lock on the client is not a good idea but unless we have some secondary way to authorize the action anyone could interfere with the update given its ID. Bill Farner wrote: That's exactly what we need. Abort/pause/resume are unscoped, big red buttons. Maxim Khutornenko wrote: So, anyone authorized to act on the job would be able to interfere? I thought the big red button would be the cancel_update with the abort/pause/resume allowed only to the update owner. In that case, I need to clear all job update apis from the Lock concept. Maxim Khutornenko wrote: Actually, do we even have to expose updateId from the startJobUpdate api? Given that anyone with the job access could act on it now, requiring to enter udpateID any time abort/resume/pause is needed feels redundant and not user friendly. Should we rather drop updateID completely and assume abort/resume/pause would attempt to act on any existing job update (if one exists)? Bill Farner wrote: Yes, i think the user-intervention functions should only be scoped by the job key. Returning the update ID from startJobUpdate is still valid for an external controller to monitor a specific update without risk of crosstalk. Maxim Khutornenko wrote: I don't see a risk of a crosstalk. Once startJobUpdate succeeds, it acquires a job lock and any attempt to call it again (e.g. race on retry) will fail. Do you have a specific scenario in mind? Back-to-back updates, or aborted then restarted update. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24744/#review50755 --- On Aug. 15, 2014, 6:14 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24744/ --- (Updated Aug. 15, 2014, 6:14 p.m.) Review request for Aurora, Mark Chu-Carroll and Bill Farner. Repository: aurora Description --- The job lock is now acquired by the startJobUpdate call. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 7ef28858ad290c74248b89c49d2a684eb1c7127e src/main/python/apache/aurora/client/api/__init__.py 62de93bb942adab47590112b76c365fac7877371 src/main/thrift/org/apache/aurora/gen/api.thrift af9f02ed1de487bc5cc2967d2edcece5b21e0be5 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 649afa24b2cfc9a1d67d350473e439d209bd720c src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 38bc9ed1ea17cac00920a2f1d066458badd7b4bb src/test/python/apache/aurora/client/api/test_api.py 96db25d9f7492a0d49e98af27f17b6cee19f5a49 src/test/python/apache/aurora/client/api/test_scheduler_client.py ab74db34d61e72c50d4ac9252b02cbf69377d194 src/test/resources/org/apache/aurora/gen/api.thrift.md5 21a05f6939da1dd7fc15cf6336bc3fee283f16ab src/test/resources/org/apache/aurora/gen/storage.thrift.md5 45762990d33969bedde7340887cde16a535e99fe Diff: https://reviews.apache.org/r/24744/diff/ Testing --- gradle -Pq build ./pants src/test/python/apache/aurora/client/api:: Thanks, Maxim Khutornenko
Re: Review Request 24744: Dropping lock from startJobUpdate parameters.
On Aug. 15, 2014, 6:20 p.m., Bill Farner wrote: src/main/thrift/org/apache/aurora/gen/api.thrift, line 751 https://reviews.apache.org/r/24744/diff/1/?file=661623#file661623line751 We really should not expose the lock. Any attempt to do anything with the lock will ~certainly interfere with the updater. Maxim Khutornenko wrote: How do we ensure pause/resume/abort is authorized to act on the update then? Sure, storing the lock on the client is not a good idea but unless we have some secondary way to authorize the action anyone could interfere with the update given its ID. Bill Farner wrote: That's exactly what we need. Abort/pause/resume are unscoped, big red buttons. Maxim Khutornenko wrote: So, anyone authorized to act on the job would be able to interfere? I thought the big red button would be the cancel_update with the abort/pause/resume allowed only to the update owner. In that case, I need to clear all job update apis from the Lock concept. Maxim Khutornenko wrote: Actually, do we even have to expose updateId from the startJobUpdate api? Given that anyone with the job access could act on it now, requiring to enter udpateID any time abort/resume/pause is needed feels redundant and not user friendly. Should we rather drop updateID completely and assume abort/resume/pause would attempt to act on any existing job update (if one exists)? Bill Farner wrote: Yes, i think the user-intervention functions should only be scoped by the job key. Returning the update ID from startJobUpdate is still valid for an external controller to monitor a specific update without risk of crosstalk. Maxim Khutornenko wrote: I don't see a risk of a crosstalk. Once startJobUpdate succeeds, it acquires a job lock and any attempt to call it again (e.g. race on retry) will fail. Do you have a specific scenario in mind? Bill Farner wrote: Back-to-back updates, or aborted then restarted update. Discussed offline. The race does not seem to be the problem here. However, we probably still want to keep the updateId in the startJobUpdate result to facilitate easier monitoring (i.e. start update - getUpdateDetails) story. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24744/#review50755 --- On Aug. 15, 2014, 6:14 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24744/ --- (Updated Aug. 15, 2014, 6:14 p.m.) Review request for Aurora, Mark Chu-Carroll and Bill Farner. Repository: aurora Description --- The job lock is now acquired by the startJobUpdate call. Diffs - src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 7ef28858ad290c74248b89c49d2a684eb1c7127e src/main/python/apache/aurora/client/api/__init__.py 62de93bb942adab47590112b76c365fac7877371 src/main/thrift/org/apache/aurora/gen/api.thrift af9f02ed1de487bc5cc2967d2edcece5b21e0be5 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 649afa24b2cfc9a1d67d350473e439d209bd720c src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 38bc9ed1ea17cac00920a2f1d066458badd7b4bb src/test/python/apache/aurora/client/api/test_api.py 96db25d9f7492a0d49e98af27f17b6cee19f5a49 src/test/python/apache/aurora/client/api/test_scheduler_client.py ab74db34d61e72c50d4ac9252b02cbf69377d194 src/test/resources/org/apache/aurora/gen/api.thrift.md5 21a05f6939da1dd7fc15cf6336bc3fee283f16ab src/test/resources/org/apache/aurora/gen/storage.thrift.md5 45762990d33969bedde7340887cde16a535e99fe Diff: https://reviews.apache.org/r/24744/diff/ Testing --- gradle -Pq build ./pants src/test/python/apache/aurora/client/api:: Thanks, Maxim Khutornenko
