Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Raft] refactor processAppendLogRequest #3435

Merged
merged 11 commits into from
Dec 27, 2021
Merged

Conversation

critical27
Copy link
Contributor

@critical27 critical27 commented Dec 8, 2021

Change notes:

  1. Main change is in processAppendLogRequest

    A brief flow as follows (not exactly same in code):
    processAppendLogRequest

    To achieve this goal, some others need to be modified:

    • In AppendLogRequest, we need to carry each log's term (which is missing previously), so as a proper iterator.
    • For similarity, we need to return committedLogId and committedLogTerm in resp. But committedLogTerm is missing previously, so I add one.
  2. We need to make sure lastLogId_ == wal_->lastLogId() in almost every case (except leader is replicating), we did this by calling wal_->rollbackToLog, since we handle AppendLog correctly, we don't need to rollback anymore, just set lastLogId_ = wal_->lastLogId() is enough.

  3. All request and resp should carry the term, step down as follower when a higher term found, otherwise we may failed to elect a leader.

  4. micor changes: make RaftPart::cleanup atomic

@codecov-commenter
Copy link

codecov-commenter commented Dec 8, 2021

Codecov Report

Merging #3435 (54341ba) into master (99f1f7a) will increase coverage by 0.00%.
The diff coverage is 74.74%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #3435    +/-   ##
========================================
  Coverage   85.19%   85.19%            
========================================
  Files        1304     1305     +1     
  Lines      121308   121417   +109     
========================================
+ Hits       103352   103445    +93     
- Misses      17956    17972    +16     
Impacted Files Coverage Δ
src/clients/storage/StorageClient.h 100.00% <ø> (ø)
src/clients/storage/StorageClientBase-inl.h 72.91% <ø> (+0.23%) ⬆️
src/graph/validator/MutateValidator.h 100.00% <ø> (ø)
src/kvstore/Listener.h 42.10% <0.00%> (ø)
src/kvstore/Part.h 100.00% <ø> (ø)
src/kvstore/raftex/RaftPart.h 98.30% <ø> (ø)
src/kvstore/raftex/test/TestShard.h 59.64% <ø> (ø)
src/storage/mutate/AddEdgesProcessor.h 100.00% <ø> (ø)
src/storage/mutate/AddVerticesProcessor.h 100.00% <ø> (ø)
src/kvstore/wal/WalFileIterator.cpp 63.47% <50.00%> (-7.53%) ⬇️
... and 48 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 92167b6...54341ba. Read the comment docs.

@critical27 critical27 removed the ready-for-testing PR: ready for the CI test label Dec 9, 2021
@critical27 critical27 force-pushed the append branch 2 times, most recently from cf1c660 to 444daea Compare December 13, 2021 03:49
@critical27
Copy link
Contributor Author

Ready to review

@critical27 critical27 added the ready-for-testing PR: ready for the CI test label Dec 18, 2021
kikimo
kikimo previously approved these changes Dec 23, 2021
Copy link
Contributor

@kikimo kikimo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

wenhaocs
wenhaocs previously approved these changes Dec 24, 2021
Copy link
Contributor

@wenhaocs wenhaocs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the general rule of setting rpc timeout?

@critical27
Copy link
Contributor Author

What's the general rule of setting rpc timeout?

Use the same raft rpc timeout is ok, previously there is a bug, so election use an another timeout.

Copy link
Contributor

@kikimo kikimo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yixinglu yixinglu merged commit 301887e into vesoft-inc:master Dec 27, 2021
@critical27 critical27 deleted the append branch December 27, 2021 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants