Dear Rakesh, It's great to see that you improve upon my suggestion! Really great. :)
Edward On Fri, Mar 21, 2014 at 4:47 AM, Rakesh R <[email protected]> wrote: > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19459/ > > On March 20th, 2014, 2:17 p.m. UTC, *Edward Ribeiro* wrote: > > > ./src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java<https://reviews.apache.org/r/19459/diff/2/?file=529349#file529349line217> > (Diff > revision 2) > > 217 > > public void unlock() throws IOException { > > I would rewrite lines 217-242 as two methods: > public void unlock() throws IOException { > boolean exceptionOccured = releaseLock(dataDirLock, "data") || > releaseLock(snapDirLock, "snap"); > if (exceptionOccured) { > throw new IOException("Failed to release ZooKeeper dir lock!"); > } > } > > private boolean releaseLock(FileLock lock, String dirName) throws > IOException { > try { > if (lock != null) { > lock.release(); > lock.channel().close(); > } > return false; > } catch (IOException ioe) { > LOG.error(String.format("Failed to release the %s directory > lock!", dirName), ioe); > return true; > } > finally { > lock = null; // it's okay to unreference even in case of failure? > } > } > > There's not much LoC savings, only a minor code repetition avoidance, so feel > free to ignore this suggestion. > > pls see the latest patch, where I've done the changes. > > > - Rakesh > > On March 20th, 2014, 6:31 p.m. UTC, Rakesh R wrote: > Review request for zookeeper, fpj, michim, Raul Gutierrez Segales, and > Camille Fournier. > By Rakesh R. > > *Updated March 20, 2014, 6:31 p.m.* > *Bugs: * ZOOKEEPER-1502<https://issues.apache.org/jira/browse/ZOOKEEPER-1502> > *Repository: * zookeeper > Description > > Prevent multiple ZooKeeper servers are using the same data directories at > same time. > Here trying to address the case where one ZK server JVM is running, what if > another server mistakenly configured the the same data dir and starts. > > Testing > > standalone tests included, quorum test case to be added. > > Diffs > > - ./src/java/main/org/apache/zookeeper/server/ZKDatabase.java (1579683) > - ./src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java > (1579683) > - > ./src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java > (1579683) > - ./src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java > (1579683) > - ./src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java > (1579683) > - ./src/java/test/org/apache/zookeeper/test/QuorumTest.java (1579683) > > View Diff <https://reviews.apache.org/r/19459/diff/> >
