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

Try to apply all committed entries in raft_periodic(). #54

Merged
merged 1 commit into from
May 16, 2018

Conversation

yossigo
Copy link
Contributor

@yossigo yossigo commented Feb 23, 2018

I have noticed that entries accumulate in the log and only get applied in the rate of calling raft_periodic(). This greatly improves write rate, and as far as I can tell should not break anything.

Perhaps a future improvement can add a cap on the number of entries (or even time) per periodic call to prevent starvation of other parts of the event loop.

@willemt willemt merged commit 8e9d6c6 into willemt:master May 16, 2018
@willemt
Copy link
Owner

willemt commented May 16, 2018

Applying one raft_apply_entry is just wrong. It's bad because:

  • We usually want to apply many entries (not one)
  • Applying multiple entries at a time is more efficient (ie. batching)

I will merge this in because of those reasons.

The one thing I am unsure about is:

  • Would a user want to apply less than all the unapplied entries?

willemt added a commit that referenced this pull request May 16, 2018
@yossigo
Copy link
Contributor Author

yossigo commented May 20, 2018

I can see why a user would want to control the rate of applying entries. However, in this case I have a feeling the proper way to allow that would be to extend the apply callback so users can indicate the call was rejected and should not be retried for some time (i.e. library needs to bail out of the apply loop).

@yossigo yossigo deleted the apply-multiple-entries branch May 24, 2018 09:25
liw referenced this pull request in liw/raft Nov 13, 2018
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