aurora-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Maxim Khutornenko" <ma...@apache.org>
Subject Re: Review Request 16268: Lock should be released if job does not exist
Date Sat, 14 Dec 2013 02:23:36 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message