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