Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/184
@anmolnar any ETA on merging this to master? I'd like to break up #627 into
several parts with associated JIRAs and start the process on getting that code
upstream, but need this code in master
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/184
@ivmaykov It was great help reviewing this huge PR. Thanks a lot!
Merged to 3.5 branch, master is ongoing. I need to rebase it first, because
Maven migration was committed in the meantime.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/184
>> I'm not a committer so my +1 doesn't count for much
It counts a lot, thanks for spending time not just review the patch but
also test it in production!
---
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/184
I'm not a committer so my +1 doesn't count for much, but this PR looks good
to me. I think we should get it in ASAP and then try to get my changes in (as
several separate PRs, I will need to
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/184
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2335/
---
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/184
@ivmaykov @hanm I'm done with the changes.
Let's see if we can get a green build.
---
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/184
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2297/
---
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/184
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2296/
---
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/184
@anmolnar you just need to copy `ZKTrustManager.java` from #627 to #184,
and remove the dependency on httpclient from build.xml and ivy.xml. And comment
out the 2 lines of code in
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/184
@anmolnar sure, will sign this off before next monday.
---
Github user asfgit commented on the issue:
https://github.com/apache/zookeeper/pull/184
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2259/
---
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/184
@eolivelli Looks like I triggered the build with my latest comment. Maybe
my comment to @hanm :)
---
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/184
@ivmaykov
*portUnification*
Let's just disable the feature completely in this patch: don't parse the
config option to be on the safe side. Rolling upgrade will be supported once we
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/184
I think as long as we keep `portUnification=false` there should not be
hangs/crashes. However, that means it's not possible to safely upgrade a
cluster from plaintext quorum to TLS quorum
Github user enixon commented on the issue:
https://github.com/apache/zookeeper/pull/184
+1 for @anmolnar 's plan
Afaik, with `portUnification=false` there shouldn't be any problems.
@ivmaykov does this work for you?
---
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/184
I like @anmolnar plan.
To mitigate the stability concerns around `UnifiedServerSocket`, we can
disable it by default. Unless I misunderstand something, when unified port is
disabled via
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/184
@anmolnar I wonder if it would be better to remove port unification /
unified server socket from #184 in that case, and commit it separately later?
It's very easy to bring down a cluster, we had
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/184
...and yes, upgrading existing clusters to SSL will not be supported until
we fully support port unification.
---
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/184
Additionally I've noticed that you haven't created a separate Jira for #621
which makes the administration of these issues quite cumbersome.
I think the following would be feasible and
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/184
@ivmaykov Very good point, I cannot tell you unfortunately. I would ask
more senior ZooKeeper committers for some background @phunt @hanm and what the
best practice would be.
I don't
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/184
@anmolnar what are the chances do you think of getting both PRs into the
3.5 release? I think if we only get in #184 and not #627, that would be a big
problem, since #184 has stability issues in
Github user enixon commented on the issue:
https://github.com/apache/zookeeper/pull/184
@anmolnar, We strongly agree that this PR should be merged. It has
performed well in our testing and has received extensive review at this point.
I'm pretty confident in its robustness.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/184
@enixon @ivmaykov I really appreciate your efforts to implement / improve
SSL for Zookeeper. It's a very important feature and a must-have for the 3.5
release.
With knowing that this PR
Github user enixon commented on the issue:
https://github.com/apache/zookeeper/pull/184
For context on the second pull request, testing at Facebook revealed a bug
in the existent form of 184. From #627 notes:
> Fixed networking issues/bugs in UnifiedServerSocket
> -
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/184
@anmolnar @breed @hanm please take a look at #627. That's my stack of
patches that we've added on top of this PR and have been testing internally at
Facebook. We think the two PRs should land
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/184
@hanm Sure thing. If you look back the comment history, you'll see that he
had a lot of comments, suggestions which I addressed and discussed. This PR is
now basically a joint effort of @afine
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/184
Sorry for lag, I am resuming my review on this.
On a side note - I remember @ivmaykov mentioned to me offline that he made
a couple of security improvements (related to port unification
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/184
@hanm @breed pinging again ...
---
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/184
@hanm @breed It'd great useful, if you guys have a chance to review this
important patch.
I think approval from a single committer would be enough to merge this
patch. (and a green build
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/184
Rebased to resolve merge conflict.
@hanm A gentle nudge that we're waiting for your review in this important
patch.
---
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/184
@anmolnar will do in next few days. Glad to see there are sustained efforts
pushing this important feature forward.
---
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/184
@ivmaykov Thanks very much for the review and the testing. I think your
functionality suggestions would be great additions to the feature. Given that
this PR is already huge and long
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/184
@anmolnar thanks for the hard work! Our plan is to run this on a real
cluster for about a month, if everything is working well, that will be a pretty
good argument for "this code is ready to be
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/184
Added the 2 main properties to JMX as read-only properties.
About renaming: looks like that these 2 properties are slightly different
from the others. Rest of the new settings have been
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/184
@ivmaykov Alright. :)
Wrapping up the outstanding items:
- config options renaming,
- exposing JMX properties
Hope I didn't miss anything.
---
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/184
@anmolnar actually I think the UnifiedSocket approach is fine for now, it
works. It can be cleaned up to use the java 8 socket API in a separate pull
request later.
---
Github user ivmaykov commented on the issue:
https://github.com/apache/zookeeper/pull/184
@anmolnar Most of the ssl-related config options start with `ssl.quorum`,
but this isn't true of `sslQuorum` and `portUnification`. What do you think
about renaming them. I'm open to suggestions
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/184
Rebased on latest branch-3.5
---
Github user afine commented on the issue:
https://github.com/apache/zookeeper/pull/184
Rebased on latest branch-3.5
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and
39 matches
Mail list logo
