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

Refactor: Improve log entry processing on startup #1248

Merged
merged 1 commit into from
Sep 14, 2024

Conversation

drmingdrmer
Copy link
Member

@drmingdrmer drmingdrmer commented Sep 14, 2024

Changelog

Refactor: Improve log entry processing on startup
  • Implement chunk-based reading of committed log entries when
    re-applying to state machine upon startup.

  • Add validation for log entry indexes, to avoid applying wrong entries
    to state machine.


This change is Reviewable

- Implement chunk-based reading of committed log entries when
  re-applying to state machine upon startup.

- Add validation for log entry indexes, to avoid applying wrong entries
  to state machine.
Copy link
Collaborator

@schreter schreter left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @drmingdrmer and @SteveLauC)


openraft/src/storage/helper.rs line 171 at r1 (raw file):

    }

    /// Read log entries from [`RaftLogReader`] in chunks, and apply them to the state machine.

Not for this change, but since it optimizes a similar thing: As already discussed, I'd actually prefer a streaming interface to the log (and replication) - with that, it's much easier to optimize on the state machine side. Of course, we need to change how the replies are sent to the client, but since this was already refactored, we can simply pass the reply sender via the stream as well (i.e., it would return a pair of log entry and reply sender). We should restart the discussion in the related issue :-).


openraft/src/storage/helper.rs line 173 at r1 (raw file):

    /// Read log entries from [`RaftLogReader`] in chunks, and apply them to the state machine.
    pub(crate) async fn reapply_committed(&mut self, mut start: u64, end: u64) -> Result<(), StorageError<C>> {
        let chunk_size = 64;

Maybe make configurable?

Also, entries can be wildly disparate in size, so some other metrics might be better (but, that's application-specific, so cannot be solved generically - see above, streaming interface would be helpful).

Copy link
Member Author

@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.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @schreter and @SteveLauC)


openraft/src/storage/helper.rs line 171 at r1 (raw file):

Previously, schreter wrote…

Not for this change, but since it optimizes a similar thing: As already discussed, I'd actually prefer a streaming interface to the log (and replication) - with that, it's much easier to optimize on the state machine side. Of course, we need to change how the replies are sent to the client, but since this was already refactored, we can simply pass the reply sender via the stream as well (i.e., it would return a pair of log entry and reply sender). We should restart the discussion in the related issue :-).

Alright. I'll create a draft PR to discuss the storage upgrade for the stream-oriented API.


openraft/src/storage/helper.rs line 173 at r1 (raw file):

Previously, schreter wrote…

Maybe make configurable?

Also, entries can be wildly disparate in size, so some other metrics might be better (but, that's application-specific, so cannot be solved generically - see above, streaming interface would be helpful).

Hm... Leave this optimization to stream API :(

@drmingdrmer drmingdrmer merged commit 5d75cf2 into databendlabs:main Sep 14, 2024
31 of 32 checks passed
@drmingdrmer drmingdrmer deleted the 157-reapply branch September 14, 2024 14:45
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