> On 2012-04-19 08:12:24, fpj wrote: > > 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.
Excellent point. I'm not totally sure, but it seems that some parameters will have to be statically configured on each server, such as the dataDir. Others are dynamic. At first stage, only the membership is dynamic but other parameters like tickTime syncLimit may become dynamic in the future. However, you are totally right in that the dynamic parameters are not limited to membership in general and I therefore changed all references to this in the patch. For example, the extension of an automatically created dynamic configuration file is now ".dynamic" instead of ".members" and the pointer to that file from the normal config file is now "dynamicConfigFile=..." instead of "memberhsipFile=..." > On 2012-04-19 08:12:24, fpj wrote: > > /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java, line 306 > > <https://reviews.apache.org/r/4729/diff/1/?file=101914#file101914line306> > > > > 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. these methods are used in QuorumPeerMain.java and tests similarly to other methods, such as setMinSessionTimeout(), and similarly they were maid public. > On 2012-04-19 08:12:24, fpj wrote: > > /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java, > > line 393 > > <https://reviews.apache.org/r/4729/diff/1/?file=101915#file101915line393> > > > > Please remove stack trace call. done > On 2012-04-19 08:12:24, fpj wrote: > > /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java, > > line 341 > > <https://reviews.apache.org/r/4729/diff/1/?file=101915#file101915line341> > > > > Please remove the stack trace call and add the exception object as the > > second parameter of the previous LOG.error statement. done > On 2012-04-19 08:12:24, fpj wrote: > > /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java, > > line 338 > > <https://reviews.apache.org/r/4729/diff/1/?file=101915#file101915line338> > > > > 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. done > On 2012-04-19 08:12:24, fpj wrote: > > /src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java, > > line 391 > > <https://reviews.apache.org/r/4729/diff/1/?file=101915#file101915line391> > > > > Same comment about catching an exception here. done > On 2012-04-19 08:12:24, fpj wrote: > > /src/java/test/org/apache/zookeeper/server/util/MembershipBCTest.java, line > > 45 > > <https://reviews.apache.org/r/4729/diff/1/?file=101924#file101924line45> > > > > 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. yep - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4729/#review7021 ----------------------------------------------------------- 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 > >
