----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58172/#review171325 -----------------------------------------------------------
Are we sure LOG.trace() is good? I would vote for DEBUG level. core/src/main/java/org/apache/oozie/service/ZKLocksService.java Line 59 (original), 57 (patched) <https://reviews.apache.org/r/58172/#comment244229> Are these used by the tests? If not, modify to private. core/src/main/java/org/apache/oozie/service/ZKLocksService.java Lines 164 (patched) <https://reviews.apache.org/r/58172/#comment244231> One thing that that makes me wonder - what if the lock already exists? What happens to newLockEntry? I think ZK client takes care of it and then gets reaped by a background thread. But still, at this point, we know that we don't need the newly created lock - is there any way to get rid ot it? core/src/test/java/org/apache/oozie/service/TestZKLocksService.java Lines 559 (patched) <https://reviews.apache.org/r/58172/#comment244230> It's good that you created tests, but do we test stuff like logging? Is it worth it? I rarely see tests that verify logger calls. Let's discuss this in private. - Peter Bacsko On ápr. 4, 2017, 10:40 de, András Piros wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58172/ > ----------------------------------------------------------- > > (Updated ápr. 4, 2017, 10:40 de) > > > Review request for oozie and Peter Bacsko. > > > Repository: oozie-git > > > Description > ------- > > Sometimes when Oozie tries to acquire lock [via > ZooKeeper](https://github.com/apache/oozie/blob/master/core/src/main/java/org/apache/oozie/service/ZKLocksService.java#L144) > or > [in-memory](https://github.com/apache/oozie/blob/master/core/src/main/java/org/apache/oozie/service/MemoryLocksService.java) > we don't get enough information whether the action succeeded or not. Also > other details like retry count, timeout applied, etc. are not logged. > Enhancing logging here would benefit debugging of lock handling within > `XCommand` instances. > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/service/MemoryLocksService.java > 2ab2abc34831daf75bf050b1e4c907afcb627978 > core/src/main/java/org/apache/oozie/service/ZKLocksService.java > 83790cf27772b106d39779d2ec5ccfa27ce52ef4 > core/src/test/java/org/apache/oozie/service/TestZKLocksService.java > d04f04e80a5a7ae9051d3a94feece82a9d460f67 > > > Diff: https://reviews.apache.org/r/58172/diff/1/ > > > Testing > ------- > > New unit test created, existing ones that matter remained running. > > > Thanks, > > András Piros > >
