ZOOKEEPER-2083 Remove deprecated class AuthFastLeaderElection #1171

rabi-kumar · 2019-12-10T14:44:18Z

As per ZOOKEEPER-2083 we need to remove class AuthFastLeaderElection.java so I made changes in Quorumpeer.java by removing two cases 1 and 2 present in createElectionAlgorithm as QuorumPeerconfig.electioalg is always 3.

Please do let me know if anything additional needs to be done.

maoling

QuorumPeerConfig#parseProperties also needs a update

if (electionAlg != 1 && electionAlg != 2 && electionAlg != 3) {
throw new ConfigException("Invalid electionAlg value. Only 1, 2, 3 are supported.");
}

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeer.java

…and change if condition in QuoromPeerConfig

asf-ci · 2019-12-11T15:14:12Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/1711/

maoling

+1 lgtm

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeer.java

…eElectionAlgorithm

HorizonNet

LGTM, pending QA

asf-ci · 2019-12-17T16:45:36Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/1747/

eolivelli

Looks good

I left a comment, please take a look

We are targeting only 3.7.0 (current master:
There is no need to port this change to 3.5 and 3.6

zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md

asf-ci · 2019-12-18T12:48:32Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build-maven/1752/

eolivelli

LGTM
I want to port this to branch-3.6

phunt

I'm +1 on removing this long deprecated (3.4.0) functionality. However it seems like we could provide more insight to anyone upgrading who is still using it?

phunt · 2020-01-05T18:16:18Z

zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md

-    Election implementation to use. A value of "1" corresponds to the
-    non-authenticated UDP-based version of fast leader election, "2"
-    corresponds to the authenticated UDP-based version of fast
-    leader election, and "3" corresponds to TCP-based version of
-    fast leader election. Currently, algorithm 3 is the default.
-    ###### Note
-    >The implementations of leader election 1, and 2 are now
-    **deprecated**. We have the intention
-    of removing them in the next release, at which point only the
-    FastLeaderElection will be available.


Wouldn't it be a good idea to note that these values have been used in prior versions (note which) and are no longer valid in the current version as they were deprecated in 3.4.0 and removed in...

Something along these lines would help people coming in with issues, ie if they were using 1 or 2 and upgraded.

@phunt I have added back the removed explanation of the election algorithm back in the documentation along with changes that you mentioned. Default value was made 3 in 3.2 as per ZOOKEEPER-499 and was deprecated in 3.4 and will be removed in 3.6.0.

phunt · 2020-01-05T18:17:05Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeer.java

@@ -1221,36 +1221,23 @@ protected Observer makeObserver(FileTxnSnapLog logFactory) throws IOException {
    }

    @SuppressWarnings("deprecation")
-    protected Election createElectionAlgorithm(int electionAlgorithm) {


why remove this entirely? Wouldn't it be a good idea to keep it available for folks interested in implementing their own?

I agree with @phunt . Let's keep this patch as simple as possible which would also make backporting easier. I would just blindly remove the option for AuthFastLeaderElection (and the class itself) making a note in documentation that it has been deprecated since 3.4.0.

phunt · 2020-01-05T18:17:37Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeer.java

-        case 1:
-            le = new AuthFastLeaderElection(this);
-            break;
-        case 2:
-            le = new AuthFastLeaderElection(this, true);
-            break;


similar to my comment above - if this were/is set wouldn't it be good to emit an error message?

Agree. Throw an UnsupportedException here instead of removing the switch.

Added back the switch statement and if cases are 1 and 2 then throw UnsupportedOperationException

phunt · 2020-01-05T18:18:17Z

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java

@@ -312,8 +312,8 @@ public void parseProperties(Properties zkProp) throws IOException, ConfigExcepti
                connectToLearnerMasterLimit = Integer.parseInt(value);
            } else if (key.equals("electionAlg")) {
                electionAlg = Integer.parseInt(value);
-                if (electionAlg != 1 && electionAlg != 2 && electionAlg != 3) {


perhaps a better location for the error..

Didn't make any changes in this part because default value of electionAlg is 3 and if other than 3 is used then configException is thrown instead made changes in QuorumPeer.createElectionAlgorithm

anmolnar

-1 because patch has already got enough votes, but I'd like to see @phunt 's comments addressed. Thanks.

…rumPeer

eolivelli · 2020-01-07T13:15:05Z

zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md

+    prior versions (3.0.0 and 3.1.0) were using algorithm 1 and 2 as well.
+    ###### Note
+    >The implementations of leader election 1, and 2 were 
+    **deprecated** in 3.4.0. We will be removing them in release of 3.6.0, 


What about:
Since 3.6.0 only FastLeaderElection is available, in case of upgrade you have to shutdown all of your servers and restart them with electionAlg=3 (or removing the line from the configuration file)

done pending QA

eolivelli

Looks great.
+1
thank you @ravowlga123

please @phunt and @anmolnar take a look

anmolnar

+1 Change looks good to me now.
The only thing I noticed in the diff that you introduced whitespace changes in QuorumPeer class. I think it would cleaner to revert the original file and make changes again. Not a big deal though.

anmolnar · 2020-01-07T22:07:43Z

@ravowlga123 Awesome. Thanks.
@phunt Are u happy with the patch now?

As per [ZOOKEEPER-2083](https://jira.apache.org/jira/browse/ZOOKEEPER-2083) we need to remove class AuthFastLeaderElection.java so I made changes in Quorumpeer.java by removing two cases 1 and 2 present in createElectionAlgorithm as QuorumPeerconfig.electioalg is always 3. Please do let me know if anything additional needs to be done. Author: ravowlga123 <ravowlga@gmail.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Andor Molnar <anmolnar@apache.org>, Patrick Hunt <phunt@apache.org> Closes #1171 from ravowlga123/ZOOKEEPER-2083 (cherry picked from commit 590e3cb) Signed-off-by: Enrico Olivelli <enrico.olivelli@diennea.com>

As per [ZOOKEEPER-2083](https://jira.apache.org/jira/browse/ZOOKEEPER-2083) we need to remove class AuthFastLeaderElection.java so I made changes in Quorumpeer.java by removing two cases 1 and 2 present in createElectionAlgorithm as QuorumPeerconfig.electioalg is always 3. Please do let me know if anything additional needs to be done. Author: ravowlga123 <ravowlga@gmail.com> Reviewers: Enrico Olivelli <eolivelli@apache.org>, Andor Molnar <anmolnar@apache.org>, Patrick Hunt <phunt@apache.org> Closes apache#1171 from ravowlga123/ZOOKEEPER-2083

ZOOKEEPER-2083 Remove deprecated class AuthFastLeaderElection

3ed1e43

maoling reviewed Dec 11, 2019

View reviewed changes

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeer.java Show resolved Hide resolved

ZOOKEEPER-2083 remove switch from QuoromPeer.createElectionAlgorithm …

08ea341

…and change if condition in QuoromPeerConfig

maoling approved these changes Dec 12, 2019

View reviewed changes

HorizonNet reviewed Dec 16, 2019

View reviewed changes

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeer.java Outdated Show resolved Hide resolved

ZOOKEEPER-2083 Make variables as final and remove argument from creat…

fdd41ab

…eElectionAlgorithm

HorizonNet approved these changes Dec 17, 2019

View reviewed changes

eolivelli requested changes Dec 17, 2019

View reviewed changes

zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md Show resolved Hide resolved

ZOOKEEPER-2083 Remove electionAlgorithm 1 and 2 from zookeeperAdmin docs

f29d3a0

eolivelli approved these changes Jan 3, 2020

View reviewed changes

phunt approved these changes Jan 5, 2020

View reviewed changes

anmolnar requested changes Jan 6, 2020

View reviewed changes

ZOOKEEPER-2083 Improve documentation add switch with exception in Quo…

5348d16

…rumPeer

eolivelli requested changes Jan 7, 2020

View reviewed changes

ZOOKEEPER-2083 Add info for restart in 3.6.0

68679fb

eolivelli approved these changes Jan 7, 2020

View reviewed changes

anmolnar approved these changes Jan 7, 2020

View reviewed changes

ZOOKEEPER-2083 Remove extra space in switch QuorumPeer.createElectionAlg

f71a179

asfgit closed this in 590e3cb Jan 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZOOKEEPER-2083 Remove deprecated class AuthFastLeaderElection #1171

ZOOKEEPER-2083 Remove deprecated class AuthFastLeaderElection #1171

rabi-kumar commented Dec 10, 2019

maoling left a comment

asf-ci commented Dec 11, 2019

maoling left a comment

HorizonNet left a comment

asf-ci commented Dec 17, 2019

eolivelli left a comment

asf-ci commented Dec 18, 2019

eolivelli left a comment

phunt left a comment

phunt Jan 5, 2020

rabi-kumar Jan 7, 2020

phunt Jan 5, 2020

anmolnar Jan 6, 2020

rabi-kumar Jan 7, 2020

phunt Jan 5, 2020

anmolnar Jan 6, 2020

rabi-kumar Jan 7, 2020

phunt Jan 5, 2020

rabi-kumar Jan 7, 2020

anmolnar left a comment

eolivelli Jan 7, 2020

rabi-kumar Jan 7, 2020

eolivelli left a comment

anmolnar left a comment

anmolnar commented Jan 7, 2020

ZOOKEEPER-2083 Remove deprecated class AuthFastLeaderElection #1171

ZOOKEEPER-2083 Remove deprecated class AuthFastLeaderElection #1171

Conversation

rabi-kumar commented Dec 10, 2019

maoling left a comment

Choose a reason for hiding this comment

asf-ci commented Dec 11, 2019

maoling left a comment

Choose a reason for hiding this comment

HorizonNet left a comment

Choose a reason for hiding this comment

asf-ci commented Dec 17, 2019

eolivelli left a comment

Choose a reason for hiding this comment

asf-ci commented Dec 18, 2019

eolivelli left a comment

Choose a reason for hiding this comment

phunt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

anmolnar left a comment

Choose a reason for hiding this comment

anmolnar commented Jan 7, 2020