From abc9cdd2865268591b7954b7fb32c201cbff9f75 Mon Sep 17 00:00:00 2001 From: jpadhiyar Date: Fri, 29 Mar 2024 20:35:17 -0300 Subject: [PATCH 1/5] refactor: Complex Condition in run method Package:com.alipay.sofa.jraft.rhea.client.failover.impl Class:FailoverClosureImpl --- .../failover/impl/FailoverClosureImpl.java | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/jraft-rheakv/rheakv-core/src/main/java/com/alipay/sofa/jraft/rhea/client/failover/impl/FailoverClosureImpl.java b/jraft-rheakv/rheakv-core/src/main/java/com/alipay/sofa/jraft/rhea/client/failover/impl/FailoverClosureImpl.java index 3c80c345e..f406e65e5 100644 --- a/jraft-rheakv/rheakv-core/src/main/java/com/alipay/sofa/jraft/rhea/client/failover/impl/FailoverClosureImpl.java +++ b/jraft-rheakv/rheakv-core/src/main/java/com/alipay/sofa/jraft/rhea/client/failover/impl/FailoverClosureImpl.java @@ -57,6 +57,23 @@ public FailoverClosureImpl(CompletableFuture future, boolean retryOnInvalidEp this.retryRunner = retryRunner; } + private boolean hasRetriesLeft(int retriesLeft) { + return retriesLeft > 0; + } + + private boolean isInvalidPeerError(Errors error) { + return ErrorsHelper.isInvalidPeer(error); + } + + private boolean isInvalidEpochError(Errors error) { + return ErrorsHelper.isInvalidEpoch(error); + } + + public boolean shouldRetryOnError(int retriesLeft, boolean retryOnInvalidEpoch, Errors error) { + return hasRetriesLeft(retriesLeft) + && (isInvalidPeerError(error) || (retryOnInvalidEpoch && isInvalidEpochError(error))); + } + @SuppressWarnings("unchecked") @Override public void run(final Status status) { @@ -66,8 +83,7 @@ public void run(final Status status) { } final Errors error = getError(); - if (this.retriesLeft > 0 - && (ErrorsHelper.isInvalidPeer(error) || (this.retryOnInvalidEpoch && ErrorsHelper.isInvalidEpoch(error)))) { + if (shouldRetryOnError(this.retriesLeft, this.retryOnInvalidEpoch, error)) { LOG.warn("[Failover] status: {}, error: {}, [{}] retries left.", status, error, this.retriesLeft); this.retryRunner.run(error); } else { From 39c185e2a863b38453efa4cfa20201ae32dbfe0e Mon Sep 17 00:00:00 2001 From: jpadhiyar Date: Sat, 30 Mar 2024 22:46:15 -0300 Subject: [PATCH 2/5] Refactor: Introduce explaining variable to make code logic more readable. --- .../com/alipay/sofa/jraft/test/atomic/HashAlgorithm.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/jraft-test/src/main/java/com/alipay/sofa/jraft/test/atomic/HashAlgorithm.java b/jraft-test/src/main/java/com/alipay/sofa/jraft/test/atomic/HashAlgorithm.java index beee0c6e5..69efd80ff 100644 --- a/jraft-test/src/main/java/com/alipay/sofa/jraft/test/atomic/HashAlgorithm.java +++ b/jraft-test/src/main/java/com/alipay/sofa/jraft/test/atomic/HashAlgorithm.java @@ -71,8 +71,11 @@ public long hash(final String k) { break; case KETAMA_HASH: byte[] bKey = computeMd5(k); - rv = (long) (bKey[3] & 0xFF) << 24 | (long) (bKey[2] & 0xFF) << 16 | (long) (bKey[1] & 0xFF) << 8 - | bKey[0] & 0xFF; + long fourthByteShifted = (long) (bKey[3] & 0xFF) << 24; // Shifted left by 24 bits + long thirdByteShifted = (long) (bKey[2] & 0xFF) << 16; // Shifted left by 16 bits + long secondByteShifted = (long) (bKey[1] & 0xFF) << 8; // Shifted left by 8 bits + long firstByte = bKey[0] & 0xFF; + rv = fourthByteShifted | thirdByteShifted | secondByteShifted | firstByte; break; } From 70f514df743fa6d6cf481777114ede444d61d0fb Mon Sep 17 00:00:00 2001 From: jpadhiyar Date: Sat, 30 Mar 2024 22:52:40 -0300 Subject: [PATCH 3/5] Refactor: Extract Method to improve code readability. Method had Cyclomatic complexity: 12 --- .../com/alipay/sofa/jraft/core/NodeImpl.java | 29 +++++++++++++------ 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/jraft-core/src/main/java/com/alipay/sofa/jraft/core/NodeImpl.java b/jraft-core/src/main/java/com/alipay/sofa/jraft/core/NodeImpl.java index ae1c0df4a..fe48943ee 100644 --- a/jraft-core/src/main/java/com/alipay/sofa/jraft/core/NodeImpl.java +++ b/jraft-core/src/main/java/com/alipay/sofa/jraft/core/NodeImpl.java @@ -1272,34 +1272,42 @@ private void stepDown(final long term, final boolean wakeupCandidate, final Stat if (!this.state.isActive()) { return; } + + handleStateSpecificActions(status); + resetLeaderIdAndState(term, status); + handleReplicatorActions(wakeupCandidate); + handleTransferActions(); + restartElectionTimer(); + } + + private void handleStateSpecificActions(final Status status) { if (this.state == State.STATE_CANDIDATE) { stopVoteTimer(); } else if (this.state.compareTo(State.STATE_TRANSFERRING) <= 0) { stopStepDownTimer(); this.ballotBox.clearPendingTasks(); - // signal fsm leader stop immediately if (this.state == State.STATE_LEADER) { onLeaderStop(status); } } - // reset leader_id - resetLeaderId(PeerId.emptyPeer(), status); + } - // soft state in memory + private void resetLeaderIdAndState(long term, Status status) { + resetLeaderId(PeerId.emptyPeer(), status); this.state = State.STATE_FOLLOWER; this.confCtx.reset(); updateLastLeaderTimestamp(Utils.monotonicMs()); if (this.snapshotExecutor != null) { this.snapshotExecutor.interruptDownloadingSnapshots(term); } - - // meta state if (term > this.currTerm) { this.currTerm = term; this.votedId = PeerId.emptyPeer(); this.metaStorage.setTermAndVotedFor(term, this.votedId); } + } + private void handleReplicatorActions(boolean wakeupCandidate) { if (wakeupCandidate) { this.wakingCandidate = this.replicatorGroup.stopAllAndFindTheNextCandidate(this.conf); if (this.wakingCandidate != null) { @@ -1308,15 +1316,18 @@ private void stepDown(final long term, final boolean wakeupCandidate, final Stat } else { this.replicatorGroup.stopAll(); } + } + + private void handleTransferActions() { if (this.stopTransferArg != null) { if (this.transferTimer != null) { this.transferTimer.cancel(true); } - // There is at most one StopTransferTimer at the same term, it's safe to - // mark stopTransferArg to NULL this.stopTransferArg = null; } - // Learner node will not trigger the election timer. + } + + private void restartElectionTimer() { if (!isLearner()) { this.electionTimer.restart(); } else { From 637409d66b0d3f9337f3b12b047e883ba18da55a Mon Sep 17 00:00:00 2001 From: jpadhiyar Date: Sat, 30 Mar 2024 22:58:59 -0300 Subject: [PATCH 4/5] Refactor: Rename Method name to more readable and understandable name. --- .../main/java/com/alipay/sofa/jraft/core/NodeImpl.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/jraft-core/src/main/java/com/alipay/sofa/jraft/core/NodeImpl.java b/jraft-core/src/main/java/com/alipay/sofa/jraft/core/NodeImpl.java index fe48943ee..e4e879b33 100644 --- a/jraft-core/src/main/java/com/alipay/sofa/jraft/core/NodeImpl.java +++ b/jraft-core/src/main/java/com/alipay/sofa/jraft/core/NodeImpl.java @@ -1743,7 +1743,7 @@ private boolean isLeaderLeaseValid() { if (checkLeaderLease(monotonicNowMs)) { return true; } - checkDeadNodes0(this.conf.getConf().getPeers(), monotonicNowMs, false, null); + checkAndUpdateLeaderWithDeadNodes(this.conf.getConf().getPeers(), monotonicNowMs, false, null); return checkLeaderLease(monotonicNowMs); } @@ -2224,7 +2224,7 @@ private boolean checkDeadNodes(final Configuration conf, final long monotonicNow // Ensure quorum nodes alive. final List peers = conf.listPeers(); final Configuration deadNodes = new Configuration(); - if (checkDeadNodes0(peers, monotonicNowMs, true, deadNodes)) { + if (checkAndUpdateLeaderWithDeadNodes(peers, monotonicNowMs, true, deadNodes)) { return true; } if (stepDownOnCheckFail) { @@ -2238,8 +2238,8 @@ private boolean checkDeadNodes(final Configuration conf, final long monotonicNow return false; } - private boolean checkDeadNodes0(final List peers, final long monotonicNowMs, final boolean checkReplicator, - final Configuration deadNodes) { + private boolean checkAndUpdateLeaderWithDeadNodes(final List peers, final long monotonicNowMs, + final boolean checkReplicator, final Configuration deadNodes) { final int leaderLeaseTimeoutMs = this.options.getLeaderLeaseTimeoutMs(); int aliveCount = 0; long startLease = Long.MAX_VALUE; From d726f779a857c653f1aafa13e4ac4aa7c25d987b Mon Sep 17 00:00:00 2001 From: jpadhiyar Date: Sun, 31 Mar 2024 19:22:41 -0300 Subject: [PATCH 5/5] cleanup --- .../com/alipay/sofa/jraft/test/atomic/HashAlgorithm.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/jraft-test/src/main/java/com/alipay/sofa/jraft/test/atomic/HashAlgorithm.java b/jraft-test/src/main/java/com/alipay/sofa/jraft/test/atomic/HashAlgorithm.java index 69efd80ff..804a99836 100644 --- a/jraft-test/src/main/java/com/alipay/sofa/jraft/test/atomic/HashAlgorithm.java +++ b/jraft-test/src/main/java/com/alipay/sofa/jraft/test/atomic/HashAlgorithm.java @@ -71,9 +71,9 @@ public long hash(final String k) { break; case KETAMA_HASH: byte[] bKey = computeMd5(k); - long fourthByteShifted = (long) (bKey[3] & 0xFF) << 24; // Shifted left by 24 bits - long thirdByteShifted = (long) (bKey[2] & 0xFF) << 16; // Shifted left by 16 bits - long secondByteShifted = (long) (bKey[1] & 0xFF) << 8; // Shifted left by 8 bits + long fourthByteShifted = (long) (bKey[3] & 0xFF) << 24; + long thirdByteShifted = (long) (bKey[2] & 0xFF) << 16; + long secondByteShifted = (long) (bKey[1] & 0xFF) << 8; long firstByte = bKey[0] & 0xFF; rv = fourthByteShifted | thirdByteShifted | secondByteShifted | firstByte; break;