You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2013/12/14 02:21:03 UTC
Review Request 16268: Lock should be released if job does not exist
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16268/
-----------------------------------------------------------
Review request for Aurora, Bill Farner and Brian Wickman.
Repository: aurora
Description
-------
Releasing the lock if the update is unable to start for any reason.
Diffs
-----
src/main/python/twitter/aurora/client/api/updater.py 32bd6a1bf6bf401b0341b6e14e570d83c3e79dd5
src/test/python/twitter/aurora/client/api/test_updater.py 0b60d9e04428eb55168e3a9c3a394f19aed2f9d2
Diff: https://reviews.apache.org/r/16268/diff/
Testing
-------
$ ./pants src/test/python/twitter/aurora/client/api:updater
Build operating on targets: OrderedSet([PythonTests(src/test/python/twitter/aurora/client/api/BUILD:updater)])
============================================================================== test session starts ===============================================================================
platform darwin -- Python 2.7.3 -- pytest-2.5.0
collected 24 items
src/test/python/twitter/aurora/client/api/test_updater.py ........................
=========================================================================== 24 passed in 0.26 seconds ============================================================================
src.test.python.twitter.aurora.client.api.updater ..... SUCCESS
Thanks,
Maxim Khutornenko
Re: Review Request 16268: Lock should be released if job does not exist
Posted by Bill Farner <wf...@apache.org>.
> On Dec. 14, 2013, 1:26 a.m., Bill Farner wrote:
> > src/main/python/twitter/aurora/client/api/updater.py, line 376
> > <https://reviews.apache.org/r/16268/diff/1/?file=397921#file397921line376>
> >
> > I think this is unsafe. 'Error' is definitely too generic to determine that it's safe to release the lock. Even recognizing that a locking-related error was encountered may not be enough, since in the future other locks could be held.
>
> Maxim Khutornenko wrote:
> The whole point of applying locks is to prevent simultaneous updates. In _get_update_instructions we are just gathering information and not applying any changes. The update has technically not started yet and no mutations were attempted. What's the point of holding the lock at this stage?
>
> Bill Farner wrote:
> I'm not arguing for holding the lock at this stage (though it does come with a check-then-act race), i'm actually arguing against releasing the lock on a generic Error.
>
> Maxim Khutornenko wrote:
> Any error coming out of that call is "harmless" as no changes have been attempted yet. Not releasing the lock here just adds unnecessary confusion on a subsequent update. Let's discuss it offline.
Closing the loop on offline discussion — the missing detail was that the client had successfully acquired the lock in order to reach this point, and has not yet made any modifications. So, releasing the lock should be safe.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16268/#review30393
-----------------------------------------------------------
On Dec. 14, 2013, 1:21 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16268/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2013, 1:21 a.m.)
>
>
> Review request for Aurora, Bill Farner and Brian Wickman.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Releasing the lock if the update is unable to start for any reason.
>
>
> Diffs
> -----
>
> src/main/python/twitter/aurora/client/api/updater.py 32bd6a1bf6bf401b0341b6e14e570d83c3e79dd5
> src/test/python/twitter/aurora/client/api/test_updater.py 0b60d9e04428eb55168e3a9c3a394f19aed2f9d2
>
> Diff: https://reviews.apache.org/r/16268/diff/
>
>
> Testing
> -------
>
> $ ./pants src/test/python/twitter/aurora/client/api:updater
> Build operating on targets: OrderedSet([PythonTests(src/test/python/twitter/aurora/client/api/BUILD:updater)])
> ============================================================================== test session starts ===============================================================================
> platform darwin -- Python 2.7.3 -- pytest-2.5.0
> collected 24 items
>
> src/test/python/twitter/aurora/client/api/test_updater.py ........................
>
> =========================================================================== 24 passed in 0.26 seconds ============================================================================
> src.test.python.twitter.aurora.client.api.updater ..... SUCCESS
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16268: Lock should be released if job does not exist
Posted by Bill Farner <wf...@apache.org>.
> On Dec. 14, 2013, 1:26 a.m., Bill Farner wrote:
> > src/main/python/twitter/aurora/client/api/updater.py, line 376
> > <https://reviews.apache.org/r/16268/diff/1/?file=397921#file397921line376>
> >
> > I think this is unsafe. 'Error' is definitely too generic to determine that it's safe to release the lock. Even recognizing that a locking-related error was encountered may not be enough, since in the future other locks could be held.
>
> Maxim Khutornenko wrote:
> The whole point of applying locks is to prevent simultaneous updates. In _get_update_instructions we are just gathering information and not applying any changes. The update has technically not started yet and no mutations were attempted. What's the point of holding the lock at this stage?
I'm not arguing for holding the lock at this stage (though it does come with a check-then-act race), i'm actually arguing against releasing the lock on a generic Error.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16268/#review30393
-----------------------------------------------------------
On Dec. 14, 2013, 1:21 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16268/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2013, 1:21 a.m.)
>
>
> Review request for Aurora, Bill Farner and Brian Wickman.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Releasing the lock if the update is unable to start for any reason.
>
>
> Diffs
> -----
>
> src/main/python/twitter/aurora/client/api/updater.py 32bd6a1bf6bf401b0341b6e14e570d83c3e79dd5
> src/test/python/twitter/aurora/client/api/test_updater.py 0b60d9e04428eb55168e3a9c3a394f19aed2f9d2
>
> Diff: https://reviews.apache.org/r/16268/diff/
>
>
> Testing
> -------
>
> $ ./pants src/test/python/twitter/aurora/client/api:updater
> Build operating on targets: OrderedSet([PythonTests(src/test/python/twitter/aurora/client/api/BUILD:updater)])
> ============================================================================== test session starts ===============================================================================
> platform darwin -- Python 2.7.3 -- pytest-2.5.0
> collected 24 items
>
> src/test/python/twitter/aurora/client/api/test_updater.py ........................
>
> =========================================================================== 24 passed in 0.26 seconds ============================================================================
> src.test.python.twitter.aurora.client.api.updater ..... SUCCESS
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16268: Lock should be released if job does not exist
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Dec. 14, 2013, 1:26 a.m., Bill Farner wrote:
> > src/main/python/twitter/aurora/client/api/updater.py, line 376
> > <https://reviews.apache.org/r/16268/diff/1/?file=397921#file397921line376>
> >
> > I think this is unsafe. 'Error' is definitely too generic to determine that it's safe to release the lock. Even recognizing that a locking-related error was encountered may not be enough, since in the future other locks could be held.
The whole point of applying locks is to prevent simultaneous updates. In _get_update_instructions we are just gathering information and not applying any changes. The update has technically not started yet and no mutations were attempted. What's the point of holding the lock at this stage?
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16268/#review30393
-----------------------------------------------------------
On Dec. 14, 2013, 1:21 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16268/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2013, 1:21 a.m.)
>
>
> Review request for Aurora, Bill Farner and Brian Wickman.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Releasing the lock if the update is unable to start for any reason.
>
>
> Diffs
> -----
>
> src/main/python/twitter/aurora/client/api/updater.py 32bd6a1bf6bf401b0341b6e14e570d83c3e79dd5
> src/test/python/twitter/aurora/client/api/test_updater.py 0b60d9e04428eb55168e3a9c3a394f19aed2f9d2
>
> Diff: https://reviews.apache.org/r/16268/diff/
>
>
> Testing
> -------
>
> $ ./pants src/test/python/twitter/aurora/client/api:updater
> Build operating on targets: OrderedSet([PythonTests(src/test/python/twitter/aurora/client/api/BUILD:updater)])
> ============================================================================== test session starts ===============================================================================
> platform darwin -- Python 2.7.3 -- pytest-2.5.0
> collected 24 items
>
> src/test/python/twitter/aurora/client/api/test_updater.py ........................
>
> =========================================================================== 24 passed in 0.26 seconds ============================================================================
> src.test.python.twitter.aurora.client.api.updater ..... SUCCESS
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16268: Lock should be released if job does not exist
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Dec. 14, 2013, 1:26 a.m., Bill Farner wrote:
> > src/main/python/twitter/aurora/client/api/updater.py, line 376
> > <https://reviews.apache.org/r/16268/diff/1/?file=397921#file397921line376>
> >
> > I think this is unsafe. 'Error' is definitely too generic to determine that it's safe to release the lock. Even recognizing that a locking-related error was encountered may not be enough, since in the future other locks could be held.
>
> Maxim Khutornenko wrote:
> The whole point of applying locks is to prevent simultaneous updates. In _get_update_instructions we are just gathering information and not applying any changes. The update has technically not started yet and no mutations were attempted. What's the point of holding the lock at this stage?
>
> Bill Farner wrote:
> I'm not arguing for holding the lock at this stage (though it does come with a check-then-act race), i'm actually arguing against releasing the lock on a generic Error.
Any error coming out of that call is "harmless" as no changes have been attempted yet. Not releasing the lock here just adds unnecessary confusion on a subsequent update. Let's discuss it offline.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16268/#review30393
-----------------------------------------------------------
On Dec. 14, 2013, 1:21 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16268/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2013, 1:21 a.m.)
>
>
> Review request for Aurora, Bill Farner and Brian Wickman.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Releasing the lock if the update is unable to start for any reason.
>
>
> Diffs
> -----
>
> src/main/python/twitter/aurora/client/api/updater.py 32bd6a1bf6bf401b0341b6e14e570d83c3e79dd5
> src/test/python/twitter/aurora/client/api/test_updater.py 0b60d9e04428eb55168e3a9c3a394f19aed2f9d2
>
> Diff: https://reviews.apache.org/r/16268/diff/
>
>
> Testing
> -------
>
> $ ./pants src/test/python/twitter/aurora/client/api:updater
> Build operating on targets: OrderedSet([PythonTests(src/test/python/twitter/aurora/client/api/BUILD:updater)])
> ============================================================================== test session starts ===============================================================================
> platform darwin -- Python 2.7.3 -- pytest-2.5.0
> collected 24 items
>
> src/test/python/twitter/aurora/client/api/test_updater.py ........................
>
> =========================================================================== 24 passed in 0.26 seconds ============================================================================
> src.test.python.twitter.aurora.client.api.updater ..... SUCCESS
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16268: Lock should be released if job does not exist
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16268/#review30393
-----------------------------------------------------------
src/main/python/twitter/aurora/client/api/updater.py
<https://reviews.apache.org/r/16268/#comment58172>
I think this is unsafe. 'Error' is definitely too generic to determine that it's safe to release the lock. Even recognizing that a locking-related error was encountered may not be enough, since in the future other locks could be held.
- Bill Farner
On Dec. 14, 2013, 1:21 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16268/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2013, 1:21 a.m.)
>
>
> Review request for Aurora, Bill Farner and Brian Wickman.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Releasing the lock if the update is unable to start for any reason.
>
>
> Diffs
> -----
>
> src/main/python/twitter/aurora/client/api/updater.py 32bd6a1bf6bf401b0341b6e14e570d83c3e79dd5
> src/test/python/twitter/aurora/client/api/test_updater.py 0b60d9e04428eb55168e3a9c3a394f19aed2f9d2
>
> Diff: https://reviews.apache.org/r/16268/diff/
>
>
> Testing
> -------
>
> $ ./pants src/test/python/twitter/aurora/client/api:updater
> Build operating on targets: OrderedSet([PythonTests(src/test/python/twitter/aurora/client/api/BUILD:updater)])
> ============================================================================== test session starts ===============================================================================
> platform darwin -- Python 2.7.3 -- pytest-2.5.0
> collected 24 items
>
> src/test/python/twitter/aurora/client/api/test_updater.py ........................
>
> =========================================================================== 24 passed in 0.26 seconds ============================================================================
> src.test.python.twitter.aurora.client.api.updater ..... SUCCESS
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 16268: Lock should be released if job does not exist
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16268/
-----------------------------------------------------------
(Updated Dec. 17, 2013, 3:22 a.m.)
Review request for Aurora, Bill Farner and Brian Wickman.
Changes
-------
Thanks. Added the comment and submitted.
Repository: aurora
Description
-------
Releasing the lock if the update is unable to start for any reason.
Diffs (updated)
-----
src/main/python/twitter/aurora/client/api/updater.py 155743fd95145dc834e8c96d6dd7238b59451c04
src/test/python/twitter/aurora/client/api/test_updater.py 0b60d9e04428eb55168e3a9c3a394f19aed2f9d2
Diff: https://reviews.apache.org/r/16268/diff/
Testing
-------
$ ./pants src/test/python/twitter/aurora/client/api:updater
Build operating on targets: OrderedSet([PythonTests(src/test/python/twitter/aurora/client/api/BUILD:updater)])
============================================================================== test session starts ===============================================================================
platform darwin -- Python 2.7.3 -- pytest-2.5.0
collected 24 items
src/test/python/twitter/aurora/client/api/test_updater.py ........................
=========================================================================== 24 passed in 0.26 seconds ============================================================================
src.test.python.twitter.aurora.client.api.updater ..... SUCCESS
Thanks,
Maxim Khutornenko
Re: Review Request 16268: Lock should be released if job does not exist
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16268/#review30476
-----------------------------------------------------------
Ship it!
src/main/python/twitter/aurora/client/api/updater.py
<https://reviews.apache.org/r/16268/#comment58302>
Please include a comment on why this is deemed safe. Since lock acquisition code is ~300 lines from here, it's not obvious why this is considered okay.
- Bill Farner
On Dec. 14, 2013, 1:21 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16268/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2013, 1:21 a.m.)
>
>
> Review request for Aurora, Bill Farner and Brian Wickman.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Releasing the lock if the update is unable to start for any reason.
>
>
> Diffs
> -----
>
> src/main/python/twitter/aurora/client/api/updater.py 32bd6a1bf6bf401b0341b6e14e570d83c3e79dd5
> src/test/python/twitter/aurora/client/api/test_updater.py 0b60d9e04428eb55168e3a9c3a394f19aed2f9d2
>
> Diff: https://reviews.apache.org/r/16268/diff/
>
>
> Testing
> -------
>
> $ ./pants src/test/python/twitter/aurora/client/api:updater
> Build operating on targets: OrderedSet([PythonTests(src/test/python/twitter/aurora/client/api/BUILD:updater)])
> ============================================================================== test session starts ===============================================================================
> platform darwin -- Python 2.7.3 -- pytest-2.5.0
> collected 24 items
>
> src/test/python/twitter/aurora/client/api/test_updater.py ........................
>
> =========================================================================== 24 passed in 0.26 seconds ============================================================================
> src.test.python.twitter.aurora.client.api.updater ..... SUCCESS
>
>
> Thanks,
>
> Maxim Khutornenko
>
>