----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63739/#review192309 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/service/ZKLocksService.java Lines 59 (patched) <https://reviews.apache.org/r/63739/#comment270389> You need `CONF_PREFIX` here. core/src/main/java/org/apache/oozie/service/ZKLocksService.java Lines 209-211 (patched) <https://reviews.apache.org/r/63739/#comment270392> While I understand this `Exception` handling is about to mimic previous behavior, I'd go on and investigate whether we can `catch` separate `Exception` subclasses, and whether it's OK to swallow these. core/src/main/java/org/apache/oozie/service/ZKLocksService.java Lines 222 (patched) <https://reviews.apache.org/r/63739/#comment270394> I find having this variable as seconds would be more flexible for the user. Currently no means of waiting e.g. 5 seconds between ZooKeeper outages. core/src/main/java/org/apache/oozie/service/ZKLocksService.java Line 215 (original), 236 (patched) <https://reviews.apache.org/r/63739/#comment270395> I'd only retry when there is a ZooKeeper connectivity issue, not on other types of `Exception`s. core/src/main/java/org/apache/oozie/service/ZKLocksService.java Lines 239 (patched) <https://reviews.apache.org/r/63739/#comment270393> Yep, blocking a thread for 30-ish minutes doesn't seem like a good idea. E.g. ZooKeeper outage lasts for only a few seconds under heavy load - it can happen that Oozie JVM ends up blocking lots of `Thread`s in `TIMED_WAITING` state. Can you think of some, more elaborate solution? Maybe having an `ExecutorService` just to try to unlock, and `Callable` instances that would try unlocking. In that case, the number of `Thread`s blocked in `TIMED_WAITING` state would be: * configurable * limited to a manageable extent core/src/test/java/org/apache/oozie/service/TestZKLocksService.java Lines 563 (patched) <https://reviews.apache.org/r/63739/#comment270396> What about `Services.get().get(ZKLocksService.class)`? core/src/test/java/org/apache/oozie/service/TestZKLocksService.java Lines 567-574 (patched) <https://reviews.apache.org/r/63739/#comment270397> Extract method `setupSilentLock()`. core/src/test/java/org/apache/oozie/service/TestZKLocksService.java Lines 579-598 (patched) <https://reviews.apache.org/r/63739/#comment270398> Extract nested class w/ a catchy name. core/src/test/java/org/apache/oozie/service/TestZKLocksService.java Lines 594 (patched) <https://reviews.apache.org/r/63739/#comment270399> `LOG.error(e)`. - András Piros On Nov. 27, 2017, 7:22 p.m., Satish Saley wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63739/ > ----------------------------------------------------------- > > (Updated Nov. 27, 2017, 7:22 p.m.) > > > Review request for oozie. > > > Bugs: OOZIE-3113 > https://issues.apache.org/jira/browse/OOZIE-3113 > > > Repository: oozie-git > > > Description > ------- > > OOZIE-3113 Retry for ZK lock release > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/service/ZKLocksService.java 2c71c001f > core/src/main/resources/oozie-default.xml 8285df0a7 > core/src/test/java/org/apache/oozie/service/TestZKLocksService.java > d04f04e80 > > > Diff: https://reviews.apache.org/r/63739/diff/2/ > > > Testing > ------- > > Tested locally > > > Thanks, > > Satish Saley > >
