Mailing-List: contact dev-help@aurora.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@aurora.incubator.apache.org
Content-Type: multipart/alternative;
 boundary="===============1361783962675890725=="
MIME-Version: 1.0
Subject: Re: Review Request 16268: Lock should be released if job does not
 exist
From: "Maxim Khutornenko" <maxim@apache.org>
To: "Bill Farner" <wfarner@apache.org>, "Brian Wickman" <wickman@gmail.com>
Cc: "Maxim Khutornenko" <maxim@apache.org>,
 "Aurora" <dev@aurora.incubator.apache.org>
Date: Sat, 14 Dec 2013 01:32:12 -0000
Message-ID: <20131214013212.27438.81320@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Maxim Khutornenko" <noreply@reviews.apache.org>
References: <20131214012650.27438.56855@reviews.apache.org>
In-Reply-To: <20131214012650.27438.56855@reviews.apache.org>
Reply-To: "Maxim Khutornenko" <maxim@apache.org>

--===============1361783962675890725==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


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


--===============1361783962675890725==--