Raft problem #2483

sherlockkenan · 2021-06-30T11:15:05Z

What changes were proposed in this pull request?

####. fix inconsistency problem in raftpart appendlog method.

*Hi, when I read the implementation of raft in neblua, I found out that may exist a inconsistency problem. In the raftpart, if a follower receive logs from leader conflicting with itself, it will rollback the log to lastCommitID. such behavior may cause inconsistency commit logs.

here is a picture to show how it may happend :

I think that the follower could not deletes the log arbitrarily. because some logs may have been already committed.

Why are the changes needed?

I refine the implementation to follow the guildline in the paper of raft .

Will break the compatibility? How if so?

no

Does this PR introduce any user-facing change?

no

How was this patch tested?

Checklist

I've run the tests to see all new and existing tests pass
If this Pull Request resolves an issue, I linked to the issue in the text above
I've informed the technical writer about the documentation change if necessary

CLAassistant · 2021-06-30T11:15:09Z

All committers have signed the CLA.

Shylock-Hg · 2021-07-01T02:58:28Z

I have a problem about the flow in above picture, in that, the leader must commit after committed by major, but the stage 1,2 doesn't.

critical27 · 2021-07-01T04:14:59Z

👍 , this is a known issue (long story). I'll review later.

sherlockkenan · 2021-07-01T04:32:10Z

I have a problem about the flow in above picture, in that, the leader must commit after committed by major, but the stage 1,2 doesn't.

the leader could commit after it sends logs to major successfully. after that leader update its commit message to followers in the next request. Before transferring the commit message, commitId between leader and followers could be different.

critical27

Thx for contribution, some place need to have a discuss, see the comments.

critical27 · 2021-07-01T09:33:07Z

src/kvstore/raftex/RaftPart.cpp

+                                            req.get_log_str_list().end());
+
+    // may be need to rollback wal_
+    if (!( req.get_last_log_id_sent() == wal_->lastLogId() && req.get_last_log_term_sent() == wal_->lastLogTerm())) {


Why we didn't check the condition like in line 1551 req.get_last_log_id_sent() == lastLogId_ && req.get_last_log_term_sent() == lastLogTerm_?

My point is, when it is true, we don't need to check term in each log according to the property of Log Matching in paper.

yes, using

req.get_last_log_id_sent() == lastLogId_ && req.get_last_log_term_sent() == lastLogTerm_

to check is better, I will modify it later.

I agree that when it is true, it not need to check term in each log. that is why I add this condition check.

if (!( req.get_last_log_id_sent() == wal_->lastLogId() && req.get_last_log_term_sent() == wal_->lastLogTerm()))

In most case, above check is false, therefore we don not need check term of log one by one, just append the new log。

In most case, above check is false, therefore we don not need check term of log one by one, just append the new log。

Agree.

The reason I think lastLogId_ and lastLogTerm_ will be better is that: If a host converts from follower -> leader -> follower again, in the leader phase, it could write some log in wal (although the case is rare).

src/kvstore/wal/FileBasedWal.cpp

critical27 · 2021-07-01T10:20:21Z

src/kvstore/raftex/RaftPart.cpp

+            TermID committedLogTerm = wal_->getLogTerm(committedLogId_);
+            if (committedLogTerm > 0 ) {
+                resp.set_last_log_id(committedLogId_);
+                resp.set_last_log_term(committedLogTerm);


Could consider do not set_last_log_term, IIRC, it is not used, so we don't nedd to wal_->getLogTerm(committedLogId_);

I have considered it before, but if we don't set it，then the resp is: (commmitedLogId_,last_log_term).

when leader receive the response, it will send a new request with (commmitedLogId_,last_log_term) not (commmitedLogId_, commmitedLogIdTerm_), see self->lastLogTermSent_ = resp.get_last_log_term();

append log request with (commmitedLogId_,last_log_term) could not pass the check in the follower , which will causing a dead loop

Yeah, you're right.

codecov-commenter · 2021-07-02T04:04:33Z

Codecov Report

Merging #2483 (06a816b) into master (2249c03) will decrease coverage by 0.23%.
The diff coverage is 82.39%.

@@            Coverage Diff             @@
##           master    #2483      +/-   ##
==========================================
- Coverage   86.46%   86.23%   -0.24%     
==========================================
  Files         649      647       -2     
  Lines       64374    67326    +2952     
==========================================
+ Hits        55662    58059    +2397     
- Misses       8712     9267     +555

Impacted Files	Coverage Δ
src/common/base/EitherOr.h	`95.69% <ø> (ø)`
src/common/base/StatusOr.h	`91.85% <ø> (ø)`
src/common/filter/geo/GeoParams.h	`100.00% <ø> (ø)`
src/common/fs/FileUtils.h	`85.71% <ø> (ø)`
src/common/hdfs/HdfsCommandHelper.cpp	`0.00% <0.00%> (-75.00%)`	⬇️
src/common/stats/StatsManager.h	`100.00% <ø> (ø)`
src/common/time/Duration.cpp	`75.86% <ø> (ø)`
src/common/time/Duration.h	`100.00% <ø> (ø)`
src/common/time/detail/TscHelper.cpp	`100.00% <ø> (ø)`
src/common/utils/NebulaKeyUtils.cpp	`92.16% <0.00%> (-5.29%)`	⬇️
... and 165 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2faf9bd...06a816b. Read the comment docs.

sherlockkenan · 2021-07-02T05:26:29Z

I only ran CTest in the centos environment , it is ok, but not in ubuntu clang-9. I don't known why the error happend.

critical27 · 2021-07-02T10:44:06Z

src/kvstore/raftex/RaftPart.cpp

+    size_t numLogs = req.get_log_str_list().size();
+    LogID firstId = req.get_last_log_id_sent() + 1;
+
+    std::vector<cpp2::LogEntry> logEntries (req.get_log_str_list().begin(),


Try use std::make_move_iterator here.

yeah, I have refined it

critical27 · 2021-07-02T10:47:50Z

src/kvstore/raftex/RaftPart.cpp

+        }
+
+        // update msg
+        logEntries = std::vector<cpp2::LogEntry> (req.get_log_str_list().begin() + diffIndex,


This could be quite expensive as well, emm, can't figure out a better way for now.

I have refined it to avoid copy

critical27 · 2021-07-02T10:53:26Z

I only ran CTest in the centos environment , it is ok, but not in ubuntu clang-9. I don't known why the error happend.

OK, leave it alone.

sherlockkenan · 2021-07-07T02:05:01Z

I only ran CTest in the centos environment , it is ok, but not in ubuntu clang-9. I don't known why the error happend.

OK, leave it alone.

Is there anything else that needs to be updated

critical27

Sorry for late response, LGTM now. Thx!

liuyu85cn

LGTM

jamieliu1023 · 2021-07-25T03:25:52Z

@sherlockkenan Thanks for your contribution to the Nebula Graph community! This is Jamie with Nebula and I'd like to email you the Nebula Contributor certificate and ship you a mug to mark this special moment. Could you please kindly reach me via jamie.liu(at)vesoft.com? Again, thanks for being a part of the Nebula community!

sherlockkenan · 2021-07-25T08:05:47Z

Hi，Jamie，I am please to accept this message xuehan ke

…

在 2021年7月25日，上午11:26，jamie.liu ***@***.***> 写道： @sherlockkenan Thanks for your contribution to the Nebula Graph community! This is Jamie with Nebula and I'd like to email you the Nebula Contributor certificate and ship you a mug to mark this special moment. Could you please kindly reach me via jamie.liu(at)vesoft.com? Again, thanks for being a part of the Nebula community! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

* Fix GraphSessionManager::addSessionCount() in multu-thread case * Fix compilation --------- Co-authored-by: Yichen Wang <[email protected]> Co-authored-by: Sophie <[email protected]>

fix inconsistency problem in appendlog

046b522

sherlockkenan force-pushed the raft_fix branch from 2bc5702 to 046b522 Compare June 30, 2021 13:18

sherlockkenan changed the title ~~fix inconsistency problem in appendlog~~ Raft prom Jun 30, 2021

sherlockkenan changed the title ~~Raft prom~~ Raft problem Jun 30, 2021

Shylock-Hg requested a review from a team July 1, 2021 02:45

jievince requested review from bright-starry-sky, critical27, darionyaphet, liuyu85cn and panda-sheep July 1, 2021 03:33

critical27 reviewed Jul 1, 2021

View reviewed changes

check memory buffer first and add test case

17290e1

critical27 added the ready-for-testing PR: ready for the CI test label Jul 2, 2021

critical27 requested a review from linkensphere201 July 2, 2021 03:35

critical27 reviewed Jul 2, 2021

View reviewed changes

refine logEntries implement

06a816b

sherlockkenan force-pushed the raft_fix branch from e32ad7f to 06a816b Compare July 5, 2021 12:46

sherlockkenan requested a review from critical27 July 5, 2021 12:49

sherlockkenan closed this Jul 7, 2021

sherlockkenan reopened this Jul 7, 2021

critical27 approved these changes Jul 13, 2021

View reviewed changes

liuyu85cn approved these changes Jul 21, 2021

View reviewed changes

jievince requested review from critical27 and removed request for darionyaphet, linkensphere201, bright-starry-sky and panda-sheep July 21, 2021 09:20

critical27 merged commit 0a3bd5b into vesoft-inc:master Jul 23, 2021

jamieliu1023 mentioned this pull request Jul 24, 2021

Weekly Report 2021-07-24 vesoft-inc/nebula-community#16

Closed

critical27 mentioned this pull request Sep 18, 2021

[Raft] rollback related optimize #2903

Closed

critical27 mentioned this pull request Oct 21, 2021

[Raft] make me crazy #3172

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raft problem #2483

Raft problem #2483

sherlockkenan commented Jun 30, 2021 •

edited

Loading

CLAassistant commented Jun 30, 2021 •

edited

Loading

Shylock-Hg commented Jul 1, 2021

critical27 commented Jul 1, 2021

sherlockkenan commented Jul 1, 2021 •

edited

Loading

critical27 left a comment

critical27 Jul 1, 2021 •

edited

Loading

sherlockkenan Jul 1, 2021

critical27 Jul 2, 2021 •

edited

Loading

sherlockkenan Jul 2, 2021

critical27 Jul 1, 2021

sherlockkenan Jul 1, 2021

critical27 Jul 2, 2021

codecov-commenter commented Jul 2, 2021 •

edited

Loading

sherlockkenan commented Jul 2, 2021

critical27 Jul 2, 2021

sherlockkenan Jul 5, 2021

critical27 Jul 2, 2021

sherlockkenan Jul 5, 2021

critical27 commented Jul 2, 2021

sherlockkenan commented Jul 7, 2021 •

edited

Loading

critical27 left a comment

liuyu85cn left a comment

jamieliu1023 commented Jul 25, 2021

sherlockkenan commented Jul 25, 2021 via email

Raft problem #2483

Raft problem #2483

Conversation

sherlockkenan commented Jun 30, 2021 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Will break the compatibility? How if so?

Does this PR introduce any user-facing change?

How was this patch tested?

Checklist

CLAassistant commented Jun 30, 2021 • edited Loading

Shylock-Hg commented Jul 1, 2021

critical27 commented Jul 1, 2021

sherlockkenan commented Jul 1, 2021 • edited Loading

critical27 left a comment

Choose a reason for hiding this comment

critical27 Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

critical27 Jul 2, 2021 • 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

codecov-commenter commented Jul 2, 2021 • edited Loading

Codecov Report

sherlockkenan commented Jul 2, 2021

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

critical27 commented Jul 2, 2021

sherlockkenan commented Jul 7, 2021 • edited Loading

critical27 left a comment

Choose a reason for hiding this comment

liuyu85cn left a comment

Choose a reason for hiding this comment

jamieliu1023 commented Jul 25, 2021

sherlockkenan commented Jul 25, 2021 via email

sherlockkenan commented Jun 30, 2021 •

edited

Loading

CLAassistant commented Jun 30, 2021 •

edited

Loading

sherlockkenan commented Jul 1, 2021 •

edited

Loading

critical27 Jul 1, 2021 •

edited

Loading

critical27 Jul 2, 2021 •

edited

Loading

codecov-commenter commented Jul 2, 2021 •

edited

Loading

sherlockkenan commented Jul 7, 2021 •

edited

Loading