----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4729/#review7021 -----------------------------------------------------------
One quick clarification. Is the reason why you want to have a separate file for membership to separate membership from the other configuration parameters? If so, by having this change in, we are saying that other parameters must be static. /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java <https://reviews.apache.org/r/4729/#comment15604> Does it need to be public? There are a number of public of methods following this one and I'm not sure they should be. /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java <https://reviews.apache.org/r/4729/#comment15603> Do we really want to catch the exception here or we want to propagate it up? It sounds like you're using the return value to indicate an error, but in my opinion we should propagate the exception instead. /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java <https://reviews.apache.org/r/4729/#comment15601> Please remove the stack trace call and add the exception object as the second parameter of the previous LOG.error statement. /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java <https://reviews.apache.org/r/4729/#comment15605> Same comment about catching an exception here. /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java <https://reviews.apache.org/r/4729/#comment15602> Please remove stack trace call. /src/java/test/org/apache/zookeeper/server/util/MembershipBCTest.java <https://reviews.apache.org/r/4729/#comment15606> Clarification. This is the only new test, so I'm assuming that forward compatibility is being tested through the other tests that are modified to work with the membership file. - fpj On 2012-04-15 20:49:35, Alexander Shraer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4729/ > ----------------------------------------------------------- > > (Updated 2012-04-15 20:49:35) > > > Review request for zookeeper. > > > Summary > ------- > > https://issues.apache.org/jira/browse/ZOOKEEPER-1411 > > Currently every server has a different configuration file. With this patch, > we will have all cluster membership definitions in a single file, and every > sever can have a copy of this file. > > > Diffs > ----- > > /src/java/main/org/apache/zookeeper/server/quorum/FastLeaderElection.java > 1305233 > /src/java/main/org/apache/zookeeper/server/quorum/Leader.java 1305233 > /src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java > 1305233 > /src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java > 1305233 > /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 1305233 > /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java > 1305233 > /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java > 1305233 > > /src/java/main/org/apache/zookeeper/server/quorum/QuorumZooKeeperServer.java > 1305233 > > /src/java/main/org/apache/zookeeper/server/quorum/flexible/QuorumHierarchical.java > 1305233 > /src/java/main/org/apache/zookeeper/server/quorum/flexible/QuorumMaj.java > 1305233 > > /src/java/main/org/apache/zookeeper/server/quorum/flexible/QuorumVerifier.java > 1305233 > /src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerMainTest.java > 1305233 > /src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java > 1305233 > /src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 1305233 > /src/java/test/org/apache/zookeeper/server/util/MembershipBCTest.java > PRE-CREATION > /src/java/test/org/apache/zookeeper/test/CnxManagerTest.java 1305233 > /src/java/test/org/apache/zookeeper/test/FLETest.java 1305233 > /src/java/test/org/apache/zookeeper/test/FLEZeroWeightTest.java 1305233 > /src/java/test/org/apache/zookeeper/test/HierarchicalQuorumTest.java > 1305233 > /src/java/test/org/apache/zookeeper/test/LENonTerminateTest.java 1305233 > /src/java/test/org/apache/zookeeper/test/ObserverTest.java 1305233 > /src/java/test/org/apache/zookeeper/test/QuorumBase.java 1305233 > /src/java/test/org/apache/zookeeper/test/QuorumUtil.java 1305233 > /src/java/test/org/apache/zookeeper/test/StandaloneTest.java 1305233 > > Diff: https://reviews.apache.org/r/4729/diff > > > Testing > ------- > > Many tests were updated to work with the new configuration format. > MembershipBCTest.java tests backward compatibility. > > > Thanks, > > Alexander > >
