> On June 16, 2016, 12:01 a.m., Patrick Hunt wrote: > > src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java, > > line 230 > > <https://reviews.apache.org/r/47354/diff/6/?file=1406659#file1406659line230> > > > > why is this debug? throwable seems pretty heavy handed? > > Rakesh R wrote: > Agreed, will change to error.
Done > On June 16, 2016, 12:01 a.m., Patrick Hunt wrote: > > src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java, line > > 172 > > <https://reviews.apache.org/r/47354/diff/6/?file=1406658#file1406658line172> > > > > should we say "client" here or "learner"? just wondering re > > usability/understandability (vs a "real" zk client) > > Rakesh R wrote: > like mentioned in jira, do we need to modify 'QuorumClient' to > 'QuorumLearner' along with this changes? > > Patrick Hunt wrote: > I think that would make it easier to understand. If we can be consistent > and also not confuse folks btw "real" clients and servers communicating with > other servers. Done. Modified all the occurances of 'quorum client' to 'quorum learner' > On June 16, 2016, 12:01 a.m., Patrick Hunt wrote: > > src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java, > > line 249 > > <https://reviews.apache.org/r/47354/diff/6/?file=1406659#file1406659line249> > > > > again, client vs learner? or some other term (peer perhaps? - as long > > as it's consistent) Done. Modified all the occurances of 'quorum client' to 'quorum learner'. Please review it again. Thanks! > On June 16, 2016, 12:01 a.m., Patrick Hunt wrote: > > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java, line 1435 > > <https://reviews.apache.org/r/47354/diff/6/?file=1406660#file1406660line1435> > > > > It seems to me that this should be info and not warning? Why is it a > > warning given you're configuring it this way? > > Rakesh R wrote: > Agreed, will change to info Done > On June 16, 2016, 12:01 a.m., Patrick Hunt wrote: > > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java, line 1461 > > <https://reviews.apache.org/r/47354/diff/6/?file=1406660#file1406660line1461> > > > > is 20 the right number? regardless it seems we have 20 embedded in a > > few places, can we make this a constant? > > Rakesh R wrote: > I will make a constant and use the same everywhere now. Its not an ideal > value, needs to find an equation for calculating default value. Done. I created a default constant variable and used. "QUORUM_CNXN_THREADS_SIZE_DEFAULT_VALUE". > On June 16, 2016, 12:01 a.m., Patrick Hunt wrote: > > src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthClient.java, > > line 125 > > <https://reviews.apache.org/r/47354/diff/6/?file=1406669#file1406669line125> > > > > print the actual value for debugging purposes. > > Rakesh R wrote: > Agreed. Done > On June 16, 2016, 12:01 a.m., Patrick Hunt wrote: > > src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuth.java, line > > 71 > > <https://reviews.apache.org/r/47354/diff/6/?file=1406665#file1406665line71> > > > > print the actual status for debugging purposes. > > Rakesh R wrote: > Agreed. Done > On June 16, 2016, 12:01 a.m., Patrick Hunt wrote: > > src/java/main/org/apache/zookeeper/util/SecurityUtils.java, line 93 > > <https://reviews.apache.org/r/47354/diff/6/?file=1406671#file1406671line93> > > > > it may not be wise to expose all of this. a different class but see > > ZOOKEEPER-2405 for some background. pull out something useful but not > > sensitive - e.g. a pricipal name or somesuch. > > Rakesh R wrote: > Good point, I will take care this. Done. LOG.debug("Added private credential to client principal name: '{}',", clientPrincipal); > On June 16, 2016, 12:01 a.m., Patrick Hunt wrote: > > src/java/main/org/apache/zookeeper/util/SecurityUtils.java, line 218 > > <https://reviews.apache.org/r/47354/diff/6/?file=1406671#file1406671line218> > > > > ditto re creds in the log > > Rakesh R wrote: > Good point, I will take care this. Done. LOG.debug("Added private credential to service principal name: '{}'," + " GSSCredential name:{}", servicePrincipalName, cred.getName()); - Rakesh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47354/#review137886 ----------------------------------------------------------- On June 5, 2016, 7:56 a.m., Rakesh R wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47354/ > ----------------------------------------------------------- > > (Updated June 5, 2016, 7:56 a.m.) > > > Review request for zookeeper, fpj, Ivan Kelly, Patrick Hunt, and Raul > Gutierrez Segales. > > > Bugs: ZOOKEEPER-1045 > https://issues.apache.org/jira/browse/ZOOKEEPER-1045 > > > Repository: zookeeper-git > > > Description > ------- > > Quorum mutual authentication using SASL mechanism - Digest/Kerberos > > > Diffs > ----- > > build.xml ab254b2 > ivy.xml 95b0e5a > src/java/main/org/apache/zookeeper/Login.java aaa220c > src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java 21ef0fa > src/java/main/org/apache/zookeeper/server/ZooKeeperSaslServer.java 71870ce > > src/java/main/org/apache/zookeeper/server/auth/SaslServerCallbackHandler.java > 2fbd6ed > src/java/main/org/apache/zookeeper/server/quorum/Leader.java 40c6748 > src/java/main/org/apache/zookeeper/server/quorum/Learner.java c73a8ee > src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java > 8a748c7 > src/java/main/org/apache/zookeeper/server/quorum/QuorumCnxManager.java > 20e5f16 > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 2f0f21b > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java > 8ae820d > src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java > e9c8007 > > src/java/main/org/apache/zookeeper/server/quorum/auth/NullQuorumAuthClient.java > PRE-CREATION > > src/java/main/org/apache/zookeeper/server/quorum/auth/NullQuorumAuthServer.java > PRE-CREATION > src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuth.java > PRE-CREATION > src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuthClient.java > PRE-CREATION > src/java/main/org/apache/zookeeper/server/quorum/auth/QuorumAuthServer.java > PRE-CREATION > src/java/main/org/apache/zookeeper/server/quorum/auth/README.md > PRE-CREATION > > src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthClient.java > PRE-CREATION > > src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthServer.java > PRE-CREATION > src/java/main/org/apache/zookeeper/util/SecurityUtils.java PRE-CREATION > src/java/test/data/kerberos/minikdc-krb5.conf PRE-CREATION > src/java/test/data/kerberos/minikdc.ldiff PRE-CREATION > src/java/test/org/apache/zookeeper/server/quorum/CnxManagerTest.java > 831d3ed > > src/java/test/org/apache/zookeeper/server/quorum/FLEBackwardElectionRoundTest.java > c1259d1 > src/java/test/org/apache/zookeeper/server/quorum/FLECompatibilityTest.java > 72e4fc9 > src/java/test/org/apache/zookeeper/server/quorum/FLEDontCareTest.java > a4c0cb0 > src/java/test/org/apache/zookeeper/server/quorum/FLELostMessageTest.java > 39a53ca > src/java/test/org/apache/zookeeper/server/quorum/LearnerTest.java 2ae57ce > src/java/test/org/apache/zookeeper/server/quorum/QuorumCnxManagerTest.java > PRE-CREATION > src/java/test/org/apache/zookeeper/server/quorum/QuorumPeerTestBase.java > ef552db > src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java ab8ce42 > > src/java/test/org/apache/zookeeper/server/quorum/auth/KerberosSecurityTestcase.java > PRE-CREATION > > src/java/test/org/apache/zookeeper/server/quorum/auth/KerberosTestUtils.java > PRE-CREATION > src/java/test/org/apache/zookeeper/server/quorum/auth/MiniKdc.java > PRE-CREATION > src/java/test/org/apache/zookeeper/server/quorum/auth/MiniKdcTest.java > PRE-CREATION > > src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumAuthTestBase.java > PRE-CREATION > > src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumAuthUpgradeTest.java > PRE-CREATION > > src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumDigestAuthTest.java > PRE-CREATION > > src/java/test/org/apache/zookeeper/server/quorum/auth/QuorumKerberosAuthTest.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/FLEPredicateTest.java 8088505 > src/zookeeper.jute 6521e54 > > Diff: https://reviews.apache.org/r/47354/diff/ > > > Testing > ------- > > Added unit test cases to verify the changes. > > > Thanks, > > Rakesh R > >
