zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From anmolnar <...@git.apache.org>
Subject [GitHub] zookeeper issue #184: ZOOKEEPER-236: SSL Support for Atomic Broadcast protoc...
Date Sat, 15 Sep 2018 11:21:44 GMT
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 is having a few bugs related to UnifiedServerSocket which are
fixed in the other PR, I would rather push this PR to be merged at the first place. Otherwise
we cannot make progress. This one is already a burden for the community to review - look at
the time when @afine submitted: March 7, 2017(!!!) and it's still not in.
    
    Weeks, months, years are passing and SSL support is still not part of our codebase. If
we start over and review another PR with 40(!) changed files (I know it overlaps, but one
cannot distinguish the changes), we'll never reach the finish line.
    
    I strongly suggest to commit this patch (I need one more committer to accept and I'll
merge it right on) and let's move on to the next PR afterwards, because it's getting out of
our hands. One step at a time that's the best way for moving forward.
    
    Please focus on this PR first. Thanks.
    
    In addition to that, I think it's better practice to separate bugfixes from new features
if feasible. It's cleaner, easier to review because of the smaller chunks of changes and easier
to backport in the future.


---

Mime
View raw message