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