LGTM. Those changes are very helpful, thanks Rakesh! Patrick
On Mon, Dec 19, 2016 at 12:04 PM, Rakesh Radhakrishnan <rake...@apache.org> wrote: > Thanks a lot Patrick Hunt for the review comments. Please take another look > at the wiki page when you get a chance. > > I've updated the wiki page addressing these,. > > 1) ===> DONE. Added JCE encryption part. > 2) ===> DONE. Corrected case. > 3) ===> DONE. Included version. > 4) ===> DONE. Corrected numbering format. > 5) ===> DONE. Added an example case to understand the tuning mechanism. > 6) ===> DONE. I've removed this part because it can be discussed separately > and added if someone has a use case. > 7) ===> DONE. Rephrased upgrade feature section > > Thanks, > Rakesh > > On Wed, Dec 14, 2016 at 9:03 AM, Patrick Hunt <ph...@apache.org> wrote: > > > Nice job Rakesh, some comments: > > > > 1) the appendix is a great idea, should be useful for many people. One > > thing I noticed > > "There is no additional dependencies needed to use SASL with Java since > it > > is part of the the Java Standard Edition." - you might want to > mention/link > > the JCE? The JVM doesn't come with very modern encryption - some of the > > distros use more strong encryption out of the box with kerberos. I've run > > into this a number of times (need to also install JCE). > > > > 2) consistently use "ZooKeeper" rather than "Zookeeper". Only noticed > this > > in a few places... > > > > 3) on client-server it would be good to mention when it was added > (3.4.0+), > > similar to what you did with 1045. > > > > 4) on "ZooKeeper SASL configurations" the numbering of the bullets starts > > at 2.1. and finishes at 2.4. I suspect the formatting didn't copy over > > quite right? > > > > 5) similar formatting issue for "# Defaulting to > > 20quorum.cnxn.threads.size=20" > > > > Can we give any insight into how this value should be set? i.e. why is 20 > > the default and when should it be raised/lowered? > > > > 6) can the doc shed any light on why we are recommending > > "javax.security.auth.useSubjectCredsOnly=false" ? I'm not familiar with > > this myself. > > > > 7) "This feature is supported in 3.4 branch" is ambiguous - perhaps > > rephrase. What "feature" are you referring to, 1045 or to rolling > upgrade? > > Also the ref to 3.4 itself is ambiguous - perhaps change to 3.4.10+? > > > > These are some minor nits, overall impressive effort -- thanks again > > Rakesh! > > > > Patrick > > > > > > > > On Tue, Dec 13, 2016 at 6:56 PM, Rakesh Radhakrishnan < > rake...@apache.org> > > wrote: > > > > > Hi All, > > > > > > I've incorporated ZK-1045 feature details into the Apache ZooKeeper > > project > > > cwiki. Since "ZooKeeper and SASL" section is quite large I've splitted > > > ZooKeeper client-server and server-server sections into sub-pages. > Please > > > read the following page, > > > > > > https://cwiki.apache.org/confluence/display/ZOOKEEPER/ > > ZooKeeper+and+SASL+ > > > authentication > > > > > > *ZooKeeper and SASL authentication* > > > > > > - Client-Server mutual authentication > > > - Server-Server mutual authentication > > > - Appendix: Kerberos, GSSAPI, SASL, and JAAS > > > > > > I have reused the content from the "Client-Server" and "Appendix" > > sections > > > from the existing page > > > https://cwiki.apache.org/confluence/display/ZOOKEEPER/ > Zookeeper+and+SASL > > > Presently I've maintained this original page as a history, probably we > > need > > > to delete this page after everyone agrees on the changes. > > > > > > Appreciate your feedback, thanks! > > > > > > Regards, > > > Rakesh > > > > > >
