RATIS-1769. Avoid changing priorities in TransferCommand unless necessary #808

kaijchen · 2023-01-18T03:26:24Z

What changes were proposed in this pull request?

This is a followup of RATIS-1762.
Try to avoid changing priorities before transfer leadership in TransferCommand.
It will fallback to "transfer leadership by changing priority" for backward compatibility.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/RATIS-1769

How was this patch tested?

Integration test: ElectionCommandIntegrationTest#testElectionTransferCommand() and ElectionCommandIntegrationTest#testElectionTransferCommandToHigherPriority()

Manually

Transfer between peers with same priority. (new feature)

$ bin/ratis sh election transfer -peers 127.0.0.1:10024,127.0.0.1:10124,127.0.0.1:11124 -address 127.0.0.1:10124
[main] INFO org.reflections.Reflections - Reflections took 122 ms to scan 1 urls, producing 5 keys and 18 values
[main] WARN org.apache.ratis.metrics.MetricRegistries - Found multiple MetricRegistries implementations: class org.apache.ratis.metrics.impl.MetricRegistriesImpl, class org.apache.ratis.metrics.dropwizard3.Dm3MetricRegistriesImpl. Using first found implementation: org.apache.ratis.metrics.impl.MetricRegistriesImpl@1e67a849
Transferring leadership to peer n1 with address 127.0.0.1:10124
Transferring leadership initiated

Backward compatibility

Ratis shell version: 3.0.0-SNAPSHOT.
Ratis server version: 2.4.1.

$ bin/ratis sh election transfer -peers 127.0.0.1:10024,127.0.0.1:10124,127.0.0.1:11124 -address 127.0.0.1:10124
[main] INFO org.reflections.Reflections - Reflections took 135 ms to scan 1 urls, producing 5 keys and 18 values
[main] WARN org.apache.ratis.metrics.MetricRegistries - Found multiple MetricRegistries implementations: class org.apache.ratis.metrics.impl.MetricRegistriesImpl, class org.apache.ratis.metrics.dropwizard3.Dm3MetricRegistriesImpl. Using first found implementation: org.apache.ratis.metrics.impl.MetricRegistriesImpl@1e67a849
Transferring leadership to peer n1 with address 127.0.0.1:10124
Changing priority of peer n1 with address 127.0.0.1:10124 to 4
Transferring leadership to peer n1 with address 127.0.0.1:10124
Changing priority of peer n1 with address 127.0.0.1:10124 to 5
Transferring leadership initiated

In case of failure

In most cases, just a retry will fix the problem. And users can also set timeout manually by -timeout option.

$ bin/ratis sh election transfer -peers 127.0.0.1:10024,127.0.0.1:10124,127.0.0.1:11124 -address 127.0.0.1:10124
[main] INFO org.reflections.Reflections - Reflections took 135 ms to scan 1 urls, producing 5 keys and 18 values
[main] WARN org.apache.ratis.metrics.MetricRegistries - Found multiple MetricRegistries implementations: class org.apache.ratis.metrics.impl.MetricRegistriesImpl, class org.apache.ratis.metrics.dropwizard3.Dm3MetricRegistriesImpl. Using first found implementation: org.apache.ratis.metrics.impl.MetricRegistriesImpl@1e67a849
Transferring leadership to peer n1 with address 127.0.0.1:10124
Failed to transfer peer n1 with address 127.0.0.1:10124: org.apache.ratis.protocol.exceptions.TransferLeadershipException: n2@group-ABB3109A44C1: Failed to transfer leadership to n1 (timed out 3000ms): current leader is n2
	at org.apache.ratis.server.impl.TransferLeadership$PendingRequest.complete(TransferLeadership.java:67)
	at org.apache.ratis.server.impl.TransferLeadership.lambda$finish$7(TransferLeadership.java:163)
	at java.util.Optional.ifPresent(Optional.java:159)
	at org.apache.ratis.server.impl.TransferLeadership.finish(TransferLeadership.java:163)
	at org.apache.ratis.server.impl.TransferLeadership.lambda$start$4(TransferLeadership.java:136)
	at org.apache.ratis.util.TimeoutTimer.lambda$onTimeout$2(TimeoutTimer.java:101)
	at org.apache.ratis.util.LogUtils.runAndLog(LogUtils.java:38)
	at org.apache.ratis.util.LogUtils$1.run(LogUtils.java:79)
	at org.apache.ratis.util.TimeoutTimer$Task.run(TimeoutTimer.java:55)
	at java.util.TimerThread.mainLoop(Timer.java:555)
	at java.util.TimerThread.run(Timer.java:505)

kaijchen · 2023-01-18T03:31:10Z

Sadly this will break backward compatibility. But version 3.0 hasn't been released, so it might be OK.

@szetszwo @codings-dan what do you think?

kaijchen · 2023-02-03T02:34:15Z

I have come up with 3 options to preserve backward compatibility.
@szetszwo @codings-dan what do you think?

Check server version first and send command accordingly.
Add a new command and deprecate the old command.
Just break the backward compatibility, since next version is 3.0.

Personally I prever the 2nd way.

codings-dan · 2023-02-03T03:11:52Z

@kaijchen Thanks for working on this. I prefer the second option, we can add a new commad to ensure compatibility.

This reverts commit 81301ae.

kaijchen · 2023-02-07T07:41:31Z

@codings-dan @szetszwo PTAL, thanks.

codings-dan

@kaijchen Thanks for working on this, could you add a unit test for the new command? see ElectionCommandIntegrationTest#testElectionTransferCommand

kaijchen · 2023-02-09T09:04:52Z

could you add a unit test for the new command? see ElectionCommandIntegrationTest#testElectionTransferCommand

Thanks @codings-dan, I have added ElectionCommandIntegrationTest#testElectionTransferLeaderCommand.

szetszwo

@kaijchen , thanks a lot for working on this. The change looks good.

We should have test to transfer a leader who does not have the highest priority. Then, it should get some exception.

kaijchen · 2023-02-10T07:07:13Z

We should have test to transfer a leader who does not have the highest priority. Then, it should get some exception.

Thanks @szetszwo for reviewing. Good idea.

szetszwo · 2023-02-14T04:49:35Z

@kaijchen , I am thinking how to deal with the existing and the new commands.

The new election transferLeader command, since it won't change priorities, actually is yielding the leadership. How about we call it election yield?
The existing election transfer command is still useful since it can transfer the leader to a lower priority peer. How about we don't deprecate it?

kaijchen · 2023-02-14T14:34:25Z

The new election transferLeader command, since it won't change priorities, actually is yielding the leadership. How about we call it election yield?

I'm OK to call it election yield, but since raft paper call it transfer leadership, users may get confused.

The existing election transfer command is still useful since it can transfer the leader to a lower priority peer. How about we don't deprecate it?

The existing election transfer is equal to peer setPriority plus election transferLeader. User can still achieve the same functionality, and the priority will be set by user explicitly (so the user knows exactly what is going on).

Based on these reasons, I chose to deprecate the old command. @szetszwo What do you think?

szetszwo · 2023-02-14T19:52:40Z

@kaijchen , I got a new idea -- we may update the election transfer as following:

Since we already have the getRaftGroup() returned from the server, we can check priorities to decide whether to call setConfiguration. It will call setConfiguration only if the transferee has smaller priority.
Then, we don't need a new command.

We do you think?

kaijchen · 2023-02-15T02:46:50Z

Then, we don't need a new command.

The old server (prior to 2.4.1) always requires the transferee to have the highest priority for transfer leadership.
We are keeping the old transfer command for backward compatibility. Is there a way to get the server version?

Since we already have the getRaftGroup() returned from the server, we can check priorities to decide whether to call setConfiguration. It will call setConfiguration only if the transferee has smaller priority.

Personally I prefer to print a message like "transferee does not have highest priority, please call setPriority first".
Or add a -force option, to set the transferee to highest priority.

Example

let's say we have a cluster like this,

Peer	A	B	C	D	E
Priority	3	2	2	1	1

New server

If we want to transfer leader from A to E, we should only raise E's priority to highest.
So if peer E is down afterwards, the old priority remains.

Peer	A	B	C	D	E
Priority	3	2	2	1	3

Old server

Since the old server requires transferee to be the only peer with highest priority.
The best we can do is:

Peer	A	B	C	D	E
Priority	3	2	2	1	4

However, the old command will reset every node's priority, and the old priority is erased.

Peer	A	B	C	D	E
Priority	1	1	1	1	2

szetszwo · 2023-02-15T09:34:24Z

@kaijchen , that's a good point. We may change the existing election transfer command as you described:

If the target peer already has the highest priority, do not call setConfiguration.
If not, call setConfiguration to change only the target peer to the highest priority.

Does it sound good?

kaijchen · 2023-02-15T09:40:26Z

If the target peer already has the highest priority, do not call setConfiguration.

If not, call setConfiguration to change only the target peer to the highest priority.

Does it sound good?

Yes. But how do we deal with old server (backward compatibility)?
For example, for new servers (with RATIS-1762), we don't need to change priority in the following situation.
But for old servers (without RATIS-1762), we have to increase target peer's priority to 2.

Peer	A	B	C	D	E
Priority	1	1	1	1	1

Can we just drop backward compatibility?
i.e. The transfer command of Ratis shell 3.0.0 won't work with Ratis 2.4.1 servers.

szetszwo · 2023-02-15T19:19:18Z

... how do we deal with old server (backward compatibility)?

@kaijchen , When a new client talking to an old server, the client in that case should get a TransferLeadershipException saying that the target server does not have the highest priority. Then, we can send the setConfiguration in that case.

kaijchen · 2023-02-16T02:10:33Z

@kaijchen , When a new client talking to an old server, the client in that case should get a TransferLeadershipException saying that the target server does not have the highest priority. Then, we can send the setConfiguration in that case.

OK, sounds good.

kaijchen · 2023-02-16T08:47:13Z

Manually test backward compatibility, Ratis shell version 3.0.0-SNAPSHOT, Ratis server version 2.4.1:

$ bin/ratis sh election transfer -peers 127.0.0.1:10024,127.0.0.1:10124,127.0.0.1:11124 -address 127.0.0.1:11124
[main] INFO org.reflections.Reflections - Reflections took 164 ms to scan 1 urls, producing 5 keys and 18 values
[main] WARN org.apache.ratis.metrics.MetricRegistries - Found multiple MetricRegistries implementations: class org.apache.ratis.metrics.impl.MetricRegistriesImpl, class org.apache.ratis.metrics.dropwizard3.Dm3MetricRegistriesImpl. Using first found implementation: org.apache.ratis.metrics.impl.MetricRegistriesImpl@1e67a849
Transferring leadership to server with address <127.0.0.1:11124> 
Changing priority of <127.0.0.1:11124> to 1
: caught an error when executing transfer: n0@group-ABB3109A44C1 refused to transfer leadership to peer n2 as it does not has highest priority 9: peers:[n0|rpc:127.0.0.1:10024|admin:|client:|dataStream:|priority:1|startupRole:FOLLOWER, n1|rpc:127.0.0.1:10124|admin:|client:|dataStream:|priority:0|startupRole:FOLLOWER, n2|rpc:127.0.0.1:11124|admin:|client:|dataStream:|priority:1|startupRole:FOLLOWER]|listeners:[], old=null
Transferring leadership to server with address <127.0.0.1:11124> 
Changing priority of <127.0.0.1:11124> to 2
: Transferring leadership initiated

kaijchen · 2023-02-16T12:14:44Z

Note: sometimes transfer leadership succeeds, but the request timed out.
This is due to the old leader hasn't received any messages from the new leader yet.
Especially in default timeout, i.e. TransferLeadershipRequest.timeout = 0,
the timeout will default to server.randomElectionTimeout().

This will lead to TransferCommand to return -1, and cause flaky test.
So is it better to set a larger default timeout?

To fix this problem, we should wait for the new leader to be elected in the shell.
But AbstractRatisCommand seems didn't expect the leader to change.

kaijchen · 2023-02-20T07:08:41Z

Note: sometimes transfer leadership succeeds, but the request timed out. This is due to the old leader hasn't received any messages from the new leader yet. Especially in default timeout, i.e. TransferLeadershipRequest.timeout = 0, the timeout will default to server.randomElectionTimeout().

This will lead to TransferCommand to return -1, and cause flaky test. So is it better to set a larger default timeout?

Set default timeout to 3s in the new mode. The default timeout in legacy mode (fallback) is kept as 60s.

To fix this problem, we should wait for the new leader to be elected in the shell. But AbstractRatisCommand seems didn't expect the leader to change.

Added TODO in comments. It's better to keep changes small in this PR.

kaijchen · 2023-02-20T07:10:27Z

@szetszwo please take another look, thanks.

szetszwo

@kaijchen , thanks for the update. Please see the comments inlined.

szetszwo · 2023-02-20T20:31:34Z

ratis-shell/src/main/java/org/apache/ratis/shell/cli/sh/election/TransferCommand.java

+    final long timeoutDefault = 3_000L;
+    // Default timeout for legacy mode matches with the legacy command (version 2.4.x and older).
+    final long timeoutLegacy = 60_000L;
+    final Optional<Long> timeout = !cl.hasOption(TIMEOUT_OPTION_NAME) ? Optional.empty() :
+        Optional.of(Long.parseLong(cl.getOptionValue(TIMEOUT_OPTION_NAME)) * 1000L);


Use TimeDuration for supporting different units such as 100ms, 1min.

final TimeDuration timeoutDefault = TimeDuration.valueOf(3, TimeUnit.SECONDS); // Default timeout for legacy mode matches with the legacy command (version 2.4.x and older). final TimeDuration timeoutLegacy = TimeDuration.valueOf(60, TimeUnit.SECONDS); final Optional<TimeDuration> timeout = !cl.hasOption(TIMEOUT_OPTION_NAME) ? Optional.empty() : Optional.of(TimeDuration.valueOf(cl.getOptionValue(TIMEOUT_OPTION_NAME), TimeUnit.SECONDS));

szetszwo · 2023-02-20T20:44:01Z

ratis-shell/src/main/java/org/apache/ratis/shell/cli/sh/election/TransferCommand.java

+        .mapToInt(RaftPeer::getPriority).max().orElse(0);
+    RaftPeer newLeader = getRaftGroup().getPeers().stream()
+        .filter(peer -> peer.getAddress().equals(strAddr)).findAny().orElse(null);
+    if (newLeader == null) {


Print an error message.

printf("Peer with address %s not found.", strAddr);

szetszwo · 2023-02-20T20:46:16Z

ratis-shell/src/main/java/org/apache/ratis/shell/cli/sh/election/TransferCommand.java

+  private Throwable tryTransfer(RaftClient client, RaftPeer newLeader, int highestPriority, long timeout) {
+    printf("Transferring leadership to server with address <%s> %n", newLeader.getAddress());
+    try {
+      // lift the current leader to the highest priority,
+      if (newLeader.getPriority() < highestPriority) {
+        setPriority(client, newLeader.getAddress(), highestPriority);
+      }
+      RaftClientReply transferLeadershipReply =
+          client.admin().transferLeadership(newLeader.getId(), timeout);
+      processReply(transferLeadershipReply, () -> "election failed");
+    } catch (Throwable t) {
+      printf("caught an error when executing transfer: %s%n", t.getMessage());
+      return t;
+    }
+    println("Transferring leadership initiated");
+    return null;
+  }


Let's return a boolean to indicate if the transfer is success.

private boolean tryTransfer(RaftClient client, RaftPeer newLeader, int highestPriority, TimeDuration timeout) throws IOException { printf("Transferring leadership to server with address <%s> %n", newLeader.getAddress()); try { // lift the current leader to the highest priority, if (newLeader.getPriority() < highestPriority) { setPriority(client, newLeader.getAddress(), highestPriority); } RaftClientReply transferLeadershipReply = client.admin().transferLeadership(newLeader.getId(), timeout.toLong(TimeUnit.MILLISECONDS)); processReply(transferLeadershipReply, () -> "election failed"); } catch (TransferLeadershipException tle) { if (tle.getMessage().contains("it does not has highest priority")) { return false; } throw tle; } println("Transferring leadership initiated"); return true; }

szetszwo · 2023-02-20T20:47:54Z

ratis-shell/src/main/java/org/apache/ratis/shell/cli/sh/election/TransferCommand.java

+      Throwable err = tryTransfer(client, newLeader, highestPriority, timeout.orElse(timeoutDefault));
+      if (err instanceof TransferLeadershipException
+          && err.getMessage().contains("it does not has highest priority")) {
+        // legacy mode, transfer leadership by setting priority.
+        err = tryTransfer(client, newLeader, highestPriority + 1, timeout.orElse(timeoutLegacy));
+      }
+      if (err != null) {
        return -1;
      }


After changing tryTransfer to return a boolean, we could make the corresponding change as below:

try (RaftClient client = RaftUtils.createClient(getRaftGroup())) { // transfer leadership if (!tryTransfer(client, newLeader, highestPriority, timeout.orElse(timeoutDefault))) { // legacy mode, transfer leadership by setting priority. tryTransfer(client, newLeader, highestPriority + 1, timeout.orElse(timeoutLegacy)); } } catch(Throwable t) { printf("Failed to transfer peer %s with address %s: ", newLeader.getId(), newLeader.getAddress()); t.printStackTrace(getPrintStream()); return -1; } return 0;

kaijchen · 2023-02-21T02:41:29Z

Thanks @szetszwo for the review, updated.

szetszwo

+1 the change looks good.

…sary (#808) (cherry picked from commit 4bb35f0)

…sary (apache#808)

RATIS-1769. Do not change priority in TransferCommand

81301ae

kaijchen force-pushed the RATIS-1769 branch from 6179749 to 81301ae Compare February 3, 2023 02:28

kaijchen marked this pull request as ready for review February 3, 2023 02:28

kaijchen added 2 commits February 7, 2023 15:20

Fork TransferCommand to TransferLeaderCommand

abf1d19

Revert "RATIS-1769. Do not change priority in TransferCommand"

08a370c

This reverts commit 81301ae.

kaijchen changed the title ~~RATIS-1769. Do not change priority in TransferCommand~~ RATIS-1769. Add TransferLeaderCommand and deprecate TransferCommand Feb 7, 2023

deprecate the old TransferCommand

d8f5cca

kaijchen force-pushed the RATIS-1769 branch from cd80498 to d8f5cca Compare February 7, 2023 07:40

codings-dan reviewed Feb 9, 2023

View reviewed changes

add testElectionTransferLeaderCommand

2d5c449

szetszwo reviewed Feb 10, 2023

View reviewed changes

kaijchen added 4 commits February 10, 2023 22:32

Add testElectionTransferLeaderCommand

c0ff6f2

Merge branch 'apache:master' into RATIS-1769

7df9d97

Add timeout option to transferLeader

97c60ac

add timeout in description

d477718

kaijchen force-pushed the RATIS-1769 branch from 42f7fbc to d477718 Compare February 13, 2023 12:13

kaijchen requested a review from szetszwo February 13, 2023 17:32

kaijchen added 4 commits February 16, 2023 10:22

Replace the old command

e302d94

set priority first if not highest

ef5c8a8

backward compatibility

9b9f400

verify priority after transfer leadership

8a359f8

kaijchen changed the title ~~RATIS-1769. Add TransferLeaderCommand and deprecate TransferCommand~~ RATIS-1769. Try to avoid changing priorities in TransferCommand Feb 16, 2023

kaijchen added 4 commits February 16, 2023 17:00

test with timeout also

474f399

improve style

3d73056

no need to keep 60s timeout

cd0ce0d

remove unused variable

7ee62de

kaijchen changed the title ~~RATIS-1769. Try to avoid changing priorities in TransferCommand~~ RATIS-1769. Avoid changing priorities in TransferCommand unless necessary Feb 16, 2023

set default timeout properly

9a1cca5

szetszwo reviewed Feb 20, 2023

View reviewed changes

kaijchen added 2 commits February 21, 2023 10:22

TimeDuration and error message

5c3995e

Return boolean in tryTransfer

65b1989

kaijchen added 2 commits February 21, 2023 11:02

check old leader's priority after transfer in test

9ec8533

Improve output and comments

13b43ed

kaijchen force-pushed the RATIS-1769 branch from 71150a1 to 13b43ed Compare February 21, 2023 05:14

Merge branch 'apache:master' into RATIS-1769

22efeb6

szetszwo approved these changes Feb 21, 2023

View reviewed changes

szetszwo merged commit 4bb35f0 into apache:master Feb 21, 2023

kaijchen deleted the RATIS-1769 branch February 22, 2023 02:15

szetszwo pushed a commit that referenced this pull request Mar 5, 2023

RATIS-1769. Avoid changing priorities in TransferCommand unless neces…

4efca78

…sary (#808) (cherry picked from commit 4bb35f0)

kuszz pushed a commit to kuszz/ratis that referenced this pull request Mar 7, 2023

RATIS-1769. Avoid changing priorities in TransferCommand unless neces…

9c96d4f

…sary (apache#808)

symious pushed a commit to symious/ratis that referenced this pull request Mar 14, 2024

RATIS-1769. Avoid changing priorities in TransferCommand unless neces…

85f446a

…sary (apache#808)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RATIS-1769. Avoid changing priorities in TransferCommand unless necessary #808

RATIS-1769. Avoid changing priorities in TransferCommand unless necessary #808

kaijchen commented Jan 18, 2023 •

edited

Loading

kaijchen commented Jan 18, 2023

kaijchen commented Feb 3, 2023

codings-dan commented Feb 3, 2023

kaijchen commented Feb 7, 2023

codings-dan left a comment

kaijchen commented Feb 9, 2023

szetszwo left a comment

kaijchen commented Feb 10, 2023

szetszwo commented Feb 14, 2023

kaijchen commented Feb 14, 2023 •

edited

Loading

szetszwo commented Feb 14, 2023

kaijchen commented Feb 15, 2023

szetszwo commented Feb 15, 2023

kaijchen commented Feb 15, 2023 •

edited

Loading

szetszwo commented Feb 15, 2023

kaijchen commented Feb 16, 2023

kaijchen commented Feb 16, 2023

kaijchen commented Feb 16, 2023

kaijchen commented Feb 20, 2023

kaijchen commented Feb 20, 2023

szetszwo left a comment

szetszwo Feb 20, 2023

szetszwo Feb 20, 2023

szetszwo Feb 20, 2023

szetszwo Feb 20, 2023

kaijchen commented Feb 21, 2023

szetszwo left a comment

RATIS-1769. Avoid changing priorities in TransferCommand unless necessary #808

RATIS-1769. Avoid changing priorities in TransferCommand unless necessary #808

Conversation

kaijchen commented Jan 18, 2023 • edited Loading

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Manually

Backward compatibility

In case of failure

kaijchen commented Jan 18, 2023

kaijchen commented Feb 3, 2023

codings-dan commented Feb 3, 2023

kaijchen commented Feb 7, 2023

codings-dan left a comment

Choose a reason for hiding this comment

kaijchen commented Feb 9, 2023

szetszwo left a comment

Choose a reason for hiding this comment

kaijchen commented Feb 10, 2023

szetszwo commented Feb 14, 2023

kaijchen commented Feb 14, 2023 • edited Loading

szetszwo commented Feb 14, 2023

kaijchen commented Feb 15, 2023

Example

New server

Old server

szetszwo commented Feb 15, 2023

kaijchen commented Feb 15, 2023 • edited Loading

szetszwo commented Feb 15, 2023

kaijchen commented Feb 16, 2023

kaijchen commented Feb 16, 2023

kaijchen commented Feb 16, 2023

kaijchen commented Feb 20, 2023

kaijchen commented Feb 20, 2023

szetszwo left a comment

Choose a reason for hiding this comment

szetszwo Feb 20, 2023

Choose a reason for hiding this comment

szetszwo Feb 20, 2023

Choose a reason for hiding this comment

szetszwo Feb 20, 2023

Choose a reason for hiding this comment

szetszwo Feb 20, 2023

Choose a reason for hiding this comment

kaijchen commented Feb 21, 2023

szetszwo left a comment

Choose a reason for hiding this comment

kaijchen commented Jan 18, 2023 •

edited

Loading

kaijchen commented Feb 14, 2023 •

edited

Loading

kaijchen commented Feb 15, 2023 •

edited

Loading