You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2014/08/15 20:14:32 UTC
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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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
>
>
Re: Review Request 24744: Dropping lock from startJobUpdate
parameters.
Posted by Mark Chu-Carroll <mc...@twopensource.com>.
-----------------------------------------------------------
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.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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.
Changes
-------
Rebasing.
Repository: aurora
Description
-------
The job lock is now acquired by the startJobUpdate call.
Diffs (updated)
-----
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.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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.
Changes
-------
CR comments.
Repository: aurora
Description
-------
The job lock is now acquired by the startJobUpdate call.
Diffs (updated)
-----
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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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.
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?
- 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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.
Posted by Bill Farner <wf...@apache.org>.
> 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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.
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)?
- 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.
Posted by Bill Farner <wf...@apache.org>.
> 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.
That's exactly what we need. Abort/pause/resume are unscoped, big red buttons.
- 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.
Posted by Bill Farner <wf...@apache.org>.
> 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)?
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.
- 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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
>
>
Re: Review Request 24744: Dropping lock from startJobUpdate
parameters.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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
>
>