Skip to content

Conversation

@ukernel
Copy link

@ukernel ukernel commented Jun 12, 2024

Checklist

  • Updated guide with pertinent info (may not always apply).
  • Squash down commits to one or two logical commits which clearly describe the work you've done.
  • Unittest is a friend:)

This change is Reviewable

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

It's a lot changes :)

Can you explain the design purpose of this PR?

And since LogEventChannel is the major component of this PR, it requires some doc comment explaining the function of it.
And test should be added to such an important component.

And I do not quite get it why do you use the notify channel for receiving callback event. It's designed to receive all of the internal event.
https://github.com/datafuselabs/openraft/blob/30cdf5fbc9275ce6213ca8f70cf37bc0c6fe957a/openraft/src/core/raft_core.rs#L201

Reviewable status: 0 of 11 files reviewed, all discussions resolved

@drmingdrmer
Copy link
Member

Closing this PR as stale. Thank you @ukernel for the contribution effort.

Context:

  • This PR is 18+ months old with unaddressed review feedback
  • The codebase has evolved significantly since then

Current state:

  • Log appending uses async IOFlushed callback mechanism (different pattern from this PR)
  • save_vote() remains synchronous by design - it's a cold path (only during elections) where the simplicity of blocking outweighs the benefit of async callback

The async I/O architecture has been addressed through other means (#724, #735, #1169, #1180). If there's renewed interest in async save_vote(), a fresh PR aligned with the current IOFlushed pattern would be more appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants