Multi-raft-group setup rebalance without PD #176

SteNicholas · 2019-06-04T08:09:30Z

Motivation:

In the case of RheaKV multi raft group, the PD module is currently used to balance leaders between all nodes in the cluster, but there are several problems:
The PD relies on the status data carried by the heartbeat of each node to analyze and then send a 'transfer_leader' command, which causes a delay.
Some users don't want to deploy a PD cluster, but still want to use multi-raft-group, which causes the raft cluster have no ability to automatically balance the leaders.
PD is a module at the RheaKV level and cannot directly support jraft-core module.

Modification:

Provide a setup rebalance mechanism at the jraft-core level, which can be called by cli service.

Result:

Fixes #107

fengjiachun · 2019-06-05T03:41:57Z

@SteNicholas 期望的是 multi raft group 的情况下这个 api 可以平衡每个节点上的 leader 数量，你的实现看起来只是对单个 group 随机 transfer 了一下 leader ？

SteNicholas · 2019-06-05T04:43:52Z

@fengjiachun 你的意思是对Multi Raft Group所有Group都要平衡每个节点上的 leader 数量吗？

fengjiachun · 2019-06-05T04:58:04Z

@fengjiachun 你的意思是对Multi Raft Group所有Group都要平衡每个节点上的 leader 数量吗？

是的，就像 Benchmark client 代码里做的那样

SteNicholas · 2019-06-08T15:31:33Z

@fengjiachun 已经修改成Multi Raft Group场景，帮忙Review。

jraft-core/src/main/java/com/alipay/sofa/jraft/core/CliServiceImpl.java

fengjiachun · 2019-06-10T04:14:08Z

jraft-core/src/main/java/com/alipay/sofa/jraft/core/CliServiceImpl.java

+        Requires.requireTrue(!groupIds.isEmpty(), "Empty group id queue");
+        Requires.requireNonNull(conf, "Null configuration");
+
+        final int groupSizePerPeer = groupIds.size() / conf.size();


conf 只 check 了是否为 null，没有 check size 是否为0

@fengjiachun 这边check configuration的size是指Peer节点数量么？

fengjiachun · 2019-06-10T04:15:09Z

jraft-core/src/main/java/com/alipay/sofa/jraft/core/CliServiceImpl.java

@@ -383,6 +387,62 @@ public Status getLeader(final String groupId, final Configuration conf, final Pe
        return getPeers(groupId, conf, true);
    }

+    @Override
+    public Status rebalance(final Queue<String> groupIds, final Configuration conf) {


建议 groupIds 改为 List，并且方法内不修改这个 list，需要 queue 方法内自己构建

@fengjiachun 我把参数类型改成List，rebalance方法内用此参数构造Deque。

fengjiachun · 2019-06-10T04:15:35Z

jraft-core/src/main/java/com/alipay/sofa/jraft/core/CliServiceImpl.java

+            if (StringUtil.isEmpty(groupId)) {
+                break;
+            }
+            PeerId leaderId = new PeerId();


final PeerId leaderId

@fengjiachun 这边把final修饰词给遗漏了，我加上。

fengjiachun · 2019-06-10T04:16:15Z

jraft-core/src/main/java/com/alipay/sofa/jraft/core/CliServiceImpl.java

+        Requires.requireNonNull(conf, "Null configuration");
+
+        final int groupSizePerPeer = groupIds.size() / conf.size();
+        final Map<PeerId, Integer> peerMap = new ConcurrentHashMap<>();


peerMap 为局部变量， CHM 没有意义

@fengjiachun 我修改成HashMap，确实没必要用ConcurrentHashMap。

jraft-core/src/main/java/com/alipay/sofa/jraft/core/CliServiceImpl.java

fengjiachun · 2019-06-10T04:29:16Z

jraft-core/src/main/java/com/alipay/sofa/jraft/core/CliServiceImpl.java

+                    continue;
+                }
+                try {
+                    transferLeader(groupId, conf, peerId);


transferLeader 失败情况下没有处理

@fengjiachun 如果transferLeader出现异常，目前我的做法是移除此节点，调用getAlivePeers方法就拿不到此PeerId节点。

我的意思是 transferLeader 需要考虑失败的情况，返回值 Status 不能忽略

fengjiachun · 2019-06-10T04:34:05Z

jraft-core/src/main/java/com/alipay/sofa/jraft/core/CliServiceImpl.java

+                continue;
+            }
+            if (size < groupSizePerPeer) {
+                peerMap.put(leaderId, size + 1);


groupSizePerPeer * size <= sum 而不是 groupSizePerPeer * size == sum

所以这里 if (size < groupSizePerPeer) 是不是有问题？

fengjiachun · 2019-06-10T04:35:48Z

jraft-core/src/test/java/com/alipay/sofa/jraft/core/CliServiceTest.java

+        assertTrue(this.cliService.rebalance(groupIds, conf).isOk());
+        cluster.waitLeader();
+        assertEquals(leader, cluster.getLeader().getNodeId().getPeerId());
+    }


需要更严谨的单测

测试不够

fengjiachun · 2019-06-10T06:01:11Z

jraft-core/src/main/java/com/alipay/sofa/jraft/core/CliServiceImpl.java

+                if (!this.cliClientService.connect(leaderId.getEndpoint())) {
+                    return new Status(-1, "Fail to init channel to leader %s", leaderId);
+                }
+                LOG.info("Group {} leader is {}", groupId, leaderId);


日志句尾加上句号吧

@fengjiachun 我把transferLeader运行日志末尾加句号。

fengjiachun · 2019-06-10T06:01:46Z

jraft-core/src/main/java/com/alipay/sofa/jraft/core/CliServiceImpl.java

+                    groupIds.add(groupId);
+                    break;
+                } catch (final Exception e) {
+                    LOG.error("Fail to transfer leader to {}", peerId);


@fengjiachun 我把transferLeader异常日志末尾加句号。

fengjiachun · 2019-06-10T06:04:17Z

也描述一下 rebalance 的具体算法或思路吧 @SteNicholas

fengjiachun · 2019-06-10T06:25:08Z

jraft-core/src/main/java/com/alipay/sofa/jraft/CliService.java

+     * Balance the number of leaders.
+     *
+     * @param groupIds the raft group id queue
+     * @param conf    current configuration


‘current configuration’ 要和上一行对齐

@fengjiachun 我修改下comment格式。

killme2008 · 2019-06-19T09:32:55Z

测试失败了，请看下。

fengjiachun · 2019-06-19T13:23:59Z

@SteNicholas 单测还是需修改下，不要在单测里复制 rebalance 方法的逻辑，应该直接验证 rebalance 方法的输出

fengjiachun · 2019-06-20T02:29:25Z

jraft-core/src/main/java/com/alipay/sofa/jraft/core/CliServiceImpl.java

+    public Status rebalance(final List<String> groupIds, final Configuration conf, final Map<String, PeerId> leaderIds) {
+        Requires.requireTrue(!groupIds.isEmpty(), "Empty group id queue");
+        Requires.requireNonNull(conf, "Null configuration");
+        Requires.requireTrue(conf.size() != 0, "No peers of configuration");


用 !conf.isEmpty() 替换 conf.size() != 0 吧

fengjiachun · 2019-06-20T02:31:18Z

jraft-core/src/main/java/com/alipay/sofa/jraft/core/CliServiceImpl.java

@@ -383,6 +384,64 @@ public Status getLeader(final String groupId, final Configuration conf, final Pe
        return getPeers(groupId, conf, true);
    }

+    @Override
+    public Status rebalance(final List<String> groupIds, final Configuration conf, final Map<String, PeerId> leaderIds) {
+        Requires.requireTrue(!groupIds.isEmpty(), "Empty group id queue");


"Empty group id queue" 这个 msg 要改下

fengjiachun · 2019-06-20T02:37:21Z

jraft-core/src/main/java/com/alipay/sofa/jraft/core/CliServiceImpl.java

+        final Map<PeerId, Integer> peerMap = new HashMap<>();
+        for (;;) {
+            final String groupId = groupDeque.poll();
+            if (StringUtil.isEmpty(groupId)) {


这个类已经引入一个 apache 的 StringUtils 了，不要再使用类似的工具类，把 StringUtil 去掉吧

fengjiachun · 2019-06-20T02:38:17Z

jraft-core/src/main/java/com/alipay/sofa/jraft/core/CliServiceImpl.java

+            try {
+                final Status status = getLeader(groupId, conf, leaderId);
+                if (!status.isOk()) {
+                    throw new Exception("No leader in group: " + groupId);


抛出 JRaftException

fengjiachun · 2019-06-20T02:41:50Z

jraft-core/src/main/java/com/alipay/sofa/jraft/core/CliServiceImpl.java

+                    continue;
+                }
+                try {
+                    transferLeader(groupId, conf, peerId);


我的意思是 transferLeader 需要考虑失败的情况，返回值 Status 不能忽略

fengjiachun · 2019-06-21T09:44:02Z

jraft-core/src/main/java/com/alipay/sofa/jraft/CliService.java

@@ -17,6 +17,8 @@
 package com.alipay.sofa.jraft;

 import java.util.List;
+import java.util.Map;
+import java.util.Queue;


多余的 import ？检查一下，ide 一般都会有告警

fengjiachun · 2019-06-21T09:44:26Z

jraft-core/src/main/java/com/alipay/sofa/jraft/core/CliServiceImpl.java

@@ -16,8 +16,7 @@
 */
 package com.alipay.sofa.jraft.core;

-import java.util.ArrayList;
-import java.util.List;
+import java.util.*;


禁止 inport *

fengjiachun · 2019-06-21T09:46:20Z

jraft-core/src/test/java/com/alipay/sofa/jraft/core/CliServiceTest.java

-import java.util.HashSet;
-import java.util.List;
-import java.util.Set;
+import java.util.*;


同上，修改一下你的 ide 设置，禁止 import *

fengjiachun · 2019-06-21T09:46:28Z

jraft-core/src/test/java/com/alipay/sofa/jraft/core/CliServiceTest.java

 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;

+import com.alipay.sofa.jraft.*;


fengjiachun · 2019-06-21T09:56:02Z

jraft-core/src/test/java/com/alipay/sofa/jraft/core/CliServiceTest.java

+        assertTrue(this.cliService.rebalance(groupIds, conf).isOk());
+        cluster.waitLeader();
+        assertEquals(leader, cluster.getLeader().getNodeId().getPeerId());
+    }


测试不够

fengjiachun · 2019-06-21T17:58:05Z

先 merge 到临时分支，一起看看怎么更好的测试

* Multi-raft-group setup rebalance without PD (#176) * Multi-raft-group setup rebalance without PD * Multi-raft-group setup rebalance without PD * Multi-raft-group setup rebalance without PD * Multi-raft-group setup rebalance without PD * multi group rebalance * multi group rebalance * improve rebalance logic * rebalance logic unittest * rebalance logic unittest * format rebalance imports * (fix) minor fix for rebalance * (fix) minor fix and more unit test * (fix) minor fix * (fix) rebalance refactoring * (fix) add rebalance log * (fix) add rebalance log

SteNicholas added 4 commits June 4, 2019 16:06

Multi-raft-group setup rebalance without PD

4e61d04

Multi-raft-group setup rebalance without PD

16d0779

Multi-raft-group setup rebalance without PD

a1f620a

Multi-raft-group setup rebalance without PD

d3589a0

SteNicholas added 2 commits June 8, 2019 20:36

multi group rebalance

28e26d3

multi group rebalance

29c7eae

fengjiachun requested changes Jun 10, 2019

View reviewed changes

fengjiachun requested a review from killme2008 June 10, 2019 04:36

fengjiachun reviewed Jun 10, 2019

View reviewed changes

improve rebalance logic

d5f5fca

fengjiachun reviewed Jun 20, 2019

View reviewed changes

SteNicholas added 2 commits June 21, 2019 08:39

rebalance logic unittest

92fddbf

rebalance logic unittest

c2ff322

fengjiachun changed the base branch from master to feat/manual-rebalance-api June 21, 2019 09:59

fengjiachun reviewed Jun 21, 2019

View reviewed changes

format rebalance imports

6916c36

fengjiachun merged commit c2e6b26 into sofastack:feat/manual-rebalance-api Jun 21, 2019

fengjiachun mentioned this pull request Aug 15, 2019

1.2.6 release #229

Closed

4 tasks

Multi-raft-group setup rebalance without PD #176

Multi-raft-group setup rebalance without PD #176

Conversation

SteNicholas commented Jun 4, 2019 • edited by fengjiachun Loading

Motivation:

Modification:

Result:

fengjiachun commented Jun 5, 2019

SteNicholas commented Jun 5, 2019

fengjiachun commented Jun 5, 2019

SteNicholas commented Jun 8, 2019

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

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

fengjiachun commented Jun 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

killme2008 commented Jun 19, 2019

fengjiachun commented Jun 19, 2019

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

fengjiachun commented Jun 21, 2019

SteNicholas commented Jun 4, 2019 •

edited by fengjiachun

Loading