Impl learner for jraft, #8 #312

killme2008 · 2019-10-24T05:57:24Z

Motivation:

Impl read-only member(learner) for jraft, #8

Modification:

Deprecated LogEntry v1 serialization.
Supports learner in configuration.
Learner configuration change.
log replication for learner.
RPC protobuf modification for learners.
Learner test
New Node APIs to query/modify learners info.
New CliService APIs to query/modify learners info.

Result:

Fixes #8 .

Don't merge this PR, it's still in development.

killme2008 · 2019-10-26T14:22:39Z

@fengjiachun @SteNicholas This PR is ready to be reviewed.

killme2008 · 2019-11-03T09:04:04Z

@fengjiachun @SteNicholas @masaimu 这个 PR 有空帮忙 review 下。

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

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

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

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

jraft-core/src/main/java/com/alipay/sofa/jraft/entity/codec/v1/V1Encoder.java

jraft-core/src/main/java/com/alipay/sofa/jraft/option/ReplicatorOptions.java

killme2008 · 2019-11-04T02:51:44Z

@fengjiachun fixed all, please review it again.

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

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

jraft-core/src/main/java/com/alipay/sofa/jraft/conf/Configuration.java

masaimu · 2019-11-04T05:58:32Z

jraft-core/src/main/java/com/alipay/sofa/jraft/rpc/impl/cli/ResetLearnersRequestProcessor.java

+import com.google.protobuf.Message;
+
+/**
+ * RemoveLearners request processor.


Reset learners request processor.

注释有误：Reset learners request processor.

fengjiachun · 2019-11-04T15:26:54Z

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

+     * @since 1.3.0
+     * @return the learners set
+     */
+    LinkedHashSet<PeerId> listLearners();


这里之所以返回 LinkedHashSet 是我理解为了保序， List 同样保序，使用 List 与 listPeers 保持风格一致，这个我来改一下吧

fengjiachun · 2019-11-04T15:30:24Z

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

+     * @since 1.3.0
+     * @return the  alive learners set
+     */
+    LinkedHashSet<PeerId> listAliveLearners();


* (feat) minor change for learners * (feat) minor change for learners * typo * typo * typo

SteNicholas · 2019-11-05T03:11:13Z

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

+     *
+     * @param groupId the raft group id
+     * @param conf    current configuration
+     * @param learners    learner peers to add


参数注释没对齐。

SteNicholas · 2019-11-05T03:11:30Z

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

+     *
+     * @param groupId the raft group id
+     * @param conf    current configuration
+     * @param learners    learner peers to remove


这个我改了

SteNicholas · 2019-11-05T03:11:50Z

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

+     *
+     * @param groupId the raft group id
+     * @param conf    current configuration
+     * @param learners    learner peers to set


这个我改了

SteNicholas · 2019-11-05T03:24:46Z

jraft-core/src/main/java/com/alipay/sofa/jraft/conf/ConfigurationEntry.java

+        // The peer set and learner set should not have intersection set.
+        Set<PeerId> intersection = listPeers();
+        intersection.retainAll(listLearners());
+        if (intersection.isEmpty()) {


这边可以直接return intersection.isEmpty()是不是更好点？

不能直接 return，因为 false 的情况需要打印日志

SteNicholas · 2019-11-05T03:26:18Z

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

+            return processLearnersOpResponse(groupId, result, "adding learners: %s", learners);
+
+        } catch (final Exception e) {
+            return new Status(-1, e.getMessage());


这边需要打印下异常日志吗？

status 包含异常信息，返回给调用者了

SteNicholas · 2019-11-05T03:27:28Z

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

+            return processLearnersOpResponse(groupId, result, "addindg learners: %s", learners);
+
+        } catch (final Exception e) {
+            return new Status(-1, e.getMessage());


status 包含异常信息，返回给调用者了

SteNicholas · 2019-11-05T03:30:52Z

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

@@ -496,7 +638,8 @@ private PeerId findTargetPeer(final PeerId self, final String groupId, final Con
            if (result instanceof GetPeersResponse) {
                final GetPeersResponse resp = (GetPeersResponse) result;
                final List<PeerId> peerIdList = new ArrayList<>();
-                for (final String peerIdStr : resp.getPeersList()) {
+                ProtocolStringList responsePeers = returnLearners ? resp.getLearnersList() : resp.getPeersList();


缺少final关键字～

这个我改过了

SteNicholas · 2019-11-05T06:46:47Z

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

@@ -845,7 +861,7 @@ public static void waitForCaughtUp(final ThreadId id, final long maxMargin, fina
    @Override
    public String toString() {
        return "Replicator [state=" + this.state + ", statInfo=" + this.statInfo + ",peerId="
-               + this.options.getPeerId() + "]";
+               + this.options.getPeerId() + ",type=" + this.options.getReplicatorType() + "]";


type前面少了个空格

我改过了

SteNicholas · 2019-11-05T07:43:05Z

jraft-core/src/main/java/com/alipay/sofa/jraft/entity/LocalFileMetaOutter.java

-            if (hasUserMeta() != other.hasUserMeta())
-                return false;
+            boolean result = true;
+            result = result && (hasUserMeta() == other.hasUserMeta());


这个地方是不是可以boolean result = hasUserMeta() == other.hasUserMeta();

这个类是 protobuf 自动生成，我们不做额外改动

SteNicholas · 2019-11-05T07:47:22Z

jraft-core/src/main/java/com/alipay/sofa/jraft/conf/Configuration.java

+     * @param learners learners
+     * @since 1.3.0
+     */
+    public Configuration(final Iterable<PeerId> conf, final Iterable<PeerId> learners) {
        for (final PeerId peer : conf) {


这边conf需要判断是否为空吗？

fengjiachun · 2019-11-05T08:27:37Z

@SteNicholas 你提的大部分意见，我昨天改过了，还得麻烦更新再看下

* 'master' of https://github.com/sofastack/sofa-jraft: (feat) Remove multiple of 10ms for windows platform. (sofastack#351) (feat) add thread pool metric report (sofastack#339) [ISSUE#347]upgrade the com.fasterxml.jackson.core:jackson-databind to version 2.9.10.1. (sofastack#352) (feat) Support readIndex from learner (sofastack#343) (feat) counter example (sofastack#318) minor fixes (sofastack#336) Impl learner for jraft, sofastack#8 (sofastack#312) (fix) Declare rpcClient to be volatile, fix sofastack#319 (sofastack#323) (feat) add current leader id to node describe (sofastack#322) (fix) fix remove metric failed on replicator destroy (sofastack#325) (fix) raft options copy (sofastack#321) (feat) contains key api (sofastack#302) (feat) add rocksdb table_format_config and statistics (sofastack#295) feat/async snapshot (sofastack#287) fix/state listener (sofastack#285)

(feat) Impl learner for jraft, #8

7025fa5

sofastack-bot bot added cla:yes size/XXL labels Oct 24, 2019

killme2008 added 4 commits October 24, 2019 19:20

(fix) test cases and minor fixes

8e5c5d4

(feat) New APIs in CliService to support learners operaton

82e085a

(feat) test cases for learners cli service

42f6d3e

(fix) remove fsms in TestCluster

b1439e2

killme2008 requested review from fengjiachun and SteNicholas October 26, 2019 14:21

killme2008 changed the title ~~[WIP] Impl learner for jraft, #8~~ Impl learner for jraft, #8 Oct 26, 2019

fengjiachun reviewed Nov 3, 2019

View reviewed changes

killme2008 added 2 commits November 4, 2019 10:48

(feat) Adds replicator type enum and some minor changes

6037a3f

(feat) rename peers to learners

a0d6b3a

(feat) forgot ReplicatorType.java

bc4c4b8

masaimu reviewed Nov 4, 2019

View reviewed changes

killme2008 force-pushed the feature/learner branch from d139e0f to dd92e39 Compare November 4, 2019 08:16

(feat) Avoid deadlock when disruptor queue is full.

b1cc39a

killme2008 force-pushed the feature/learner branch from dd92e39 to b1cc39a Compare November 4, 2019 08:21

(feat) minor changes by CR.

9450929

fengjiachun reviewed Nov 4, 2019

View reviewed changes

masaimu self-requested a review November 5, 2019 02:41

feat/minor change learners (#332)

4ce9233

* (feat) minor change for learners * (feat) minor change for learners * typo * typo * typo

SteNicholas reviewed Nov 5, 2019

View reviewed changes

for CR

9dbfa56

masaimu approved these changes Nov 5, 2019

View reviewed changes

fengjiachun merged commit 0fdb2af into master Nov 5, 2019

fengjiachun deleted the feature/learner branch November 5, 2019 13:02

fengjiachun mentioned this pull request Nov 29, 2019

1.3.0 release #362

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Impl learner for jraft, #8 #312

Impl learner for jraft, #8 #312

killme2008 commented Oct 24, 2019 •

edited by masaimu

Loading

killme2008 commented Oct 26, 2019

killme2008 commented Nov 3, 2019

killme2008 commented Nov 4, 2019

masaimu Nov 4, 2019

masaimu Nov 5, 2019 •

edited

Loading

fengjiachun Nov 4, 2019

fengjiachun Nov 4, 2019

SteNicholas Nov 5, 2019

SteNicholas Nov 5, 2019

fengjiachun Nov 5, 2019

SteNicholas Nov 5, 2019

fengjiachun Nov 5, 2019

SteNicholas Nov 5, 2019

fengjiachun Nov 5, 2019

SteNicholas Nov 5, 2019

fengjiachun Nov 5, 2019

SteNicholas Nov 5, 2019

fengjiachun Nov 5, 2019

SteNicholas Nov 5, 2019

fengjiachun Nov 5, 2019

SteNicholas Nov 5, 2019

fengjiachun Nov 5, 2019

SteNicholas Nov 5, 2019

fengjiachun Nov 5, 2019

SteNicholas Nov 5, 2019

fengjiachun Nov 5, 2019

fengjiachun commented Nov 5, 2019

Impl learner for jraft, #8 #312

Impl learner for jraft, #8 #312

Conversation

killme2008 commented Oct 24, 2019 • edited by masaimu Loading

Motivation:

Modification:

Result:

killme2008 commented Oct 26, 2019

killme2008 commented Nov 3, 2019

killme2008 commented Nov 4, 2019

Choose a reason for hiding this comment

masaimu Nov 5, 2019 • edited Loading

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

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 Nov 5, 2019

killme2008 commented Oct 24, 2019 •

edited by masaimu

Loading

masaimu Nov 5, 2019 •

edited

Loading