> On April 7, 2017, 10:40 a.m., Peter Bacsko wrote: > > Are we sure LOG.trace() is good? I would vote for DEBUG level.
Done. > On April 7, 2017, 10:40 a.m., Peter Bacsko wrote: > > core/src/main/java/org/apache/oozie/service/ZKLocksService.java > > Line 59 (original), 57 (patched) > > <https://reviews.apache.org/r/58172/diff/1/?file=1684293#file1684293line60> > > > > Are these used by the tests? If not, modify to private. Yes, it's used in multiple other class files in the same package. > On April 7, 2017, 10:40 a.m., Peter Bacsko wrote: > > core/src/main/java/org/apache/oozie/service/ZKLocksService.java > > Lines 164 (patched) > > <https://reviews.apache.org/r/58172/diff/1/?file=1684293#file1684293line167> > > > > 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? > > Peter Bacsko wrote: > Quickly opened the JavaDoc of Curator. Looks like we cannot explicitly > destroy the lock. I think it's worth adding a single comment that having > newLockEntry is not a problem, like: > > > // We can't destoy newLockEntry and we don't have to > > This way we let other developers know that this is not a result of us > being forgetful:) Done. > On April 7, 2017, 10:40 a.m., Peter Bacsko wrote: > > core/src/test/java/org/apache/oozie/service/TestZKLocksService.java > > Lines 559 (patched) > > <https://reviews.apache.org/r/58172/diff/1/?file=1684294#file1684294line559> > > > > 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. As this patch is explicitly created for improving logging, I think it's worth to test exactly the logging behavior. - András ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58172/#review171325 ----------------------------------------------------------- On April 4, 2017, 10:40 a.m., András Piros wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58172/ > ----------------------------------------------------------- > > (Updated April 4, 2017, 10:40 a.m.) > > > 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 > >
