You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Rakesh R <ra...@huawei.com> on 2014/03/20 11:58:16 UTC
Review Request 19459: Prevent multiple zookeeper servers from using the same
data directory
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19459/
-----------------------------------------------------------
Review request for zookeeper, fpj, michim, Raul Gutierrez Segales, and Camille Fournier.
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.
Diffs
-----
./src/java/main/org/apache/zookeeper/server/ZKDatabase.java 1579527
./src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 1579527
./src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java 1579527
./src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1579527
./src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 1579527
Diff: https://reviews.apache.org/r/19459/diff/
Testing
-------
standalone tests included, quorum test case to be added.
Thanks,
Rakesh R
Re: Review Request 19459: Prevent multiple zookeeper servers from
using the same data directory
Posted by Edward Ribeiro <ed...@gmail.com>.
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 <ra...@huawei.com> 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/>
>
Re: Review Request 19459: Prevent multiple zookeeper servers from using the
same data directory
Posted by Rakesh R <ra...@huawei.com>.
> On March 20, 2014, 2:17 p.m., Edward Ribeiro wrote:
> > ./src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java, line 217
> > <https://reviews.apache.org/r/19459/diff/2/?file=529349#file529349line217>
> >
> > 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
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19459/#review37888
-----------------------------------------------------------
On March 20, 2014, 6:31 p.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19459/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, 6:31 p.m.)
>
>
> Review request for zookeeper, fpj, michim, Raul Gutierrez Segales, and Camille Fournier.
>
>
> 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.
>
>
> 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
>
> Diff: https://reviews.apache.org/r/19459/diff/
>
>
> Testing
> -------
>
> standalone tests included, quorum test case to be added.
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 19459: Prevent multiple zookeeper servers from using the
same data directory
Posted by Edward Ribeiro <ed...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19459/#review37888
-----------------------------------------------------------
./src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java
<https://reviews.apache.org/r/19459/#comment69665>
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.
- Edward Ribeiro
On March 20, 2014, 10:58 a.m., Rakesh R wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19459/
> -----------------------------------------------------------
>
> (Updated March 20, 2014, 10:58 a.m.)
>
>
> Review request for zookeeper, fpj, michim, Raul Gutierrez Segales, and Camille Fournier.
>
>
> 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.
>
>
> Diffs
> -----
>
> ./src/java/main/org/apache/zookeeper/server/ZKDatabase.java 1579527
> ./src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 1579527
> ./src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java 1579527
> ./src/java/test/org/apache/zookeeper/server/ZooKeeperServerMainTest.java 1579527
> ./src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 1579527
>
> Diff: https://reviews.apache.org/r/19459/diff/
>
>
> Testing
> -------
>
> standalone tests included, quorum test case to be added.
>
>
> Thanks,
>
> Rakesh R
>
>
Re: Review Request 19459: Prevent multiple zookeeper servers from using the
same data directory
Posted by Rakesh R <ra...@huawei.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19459/
-----------------------------------------------------------
(Updated March 20, 2014, 6:31 p.m.)
Review request for zookeeper, fpj, michim, Raul Gutierrez Segales, and Camille Fournier.
Changes
-------
Thank you Edward for the reviews. New patch contains the following changes
1) modified unlock() by using releaseLock(dir). Used | operator, release lock of both the dirs irrespective of ioexceptions
2) modified few logs
3) added two more quorum tests
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.
Diffs (updated)
-----
./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
Diff: https://reviews.apache.org/r/19459/diff/
Testing
-------
standalone tests included, quorum test case to be added.
Thanks,
Rakesh R