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: Remove RaftLogReader trait dependency from RaftLogStorage #1153

Conversation

drmingdrmer
Copy link
Member

@drmingdrmer drmingdrmer commented Jul 6, 2024

Changelog

Refactor: Remove RaftLogReader trait dependency from RaftLogStorage

This commit removes the dependency of the RaftLogReader trait from the
RaftLogStorage interface in Openraft.

trait RaftLogStorage<C>: RaftLogReader<C> + ... becomes
trait RaftLogStorage<C>: ....

This change reflects the infrequent need for reading log entries
directly from storage, as it primarily occurs when applying logs to the
state machine.

Inside Openraft, logs can now be read by acquiring a reader via
RaftLogStorage.get_log_reader().await.xxx(), rather than requiring
RaftLogStorage to implement the RaftLogReader trait.

Upgrade(non-breaking) tip:

Implementations are advised (though it is not mandatory) to remove the
impl RaftLogReader for YourRaftLogStore from their codebase.


This change is Reviewable

This commit removes the dependency of the `RaftLogReader` trait from the
`RaftLogStorage` interface in Openraft.

`trait RaftLogStorage<C>: RaftLogReader<C> + ...` becomes
`trait RaftLogStorage<C>: ...`.

This change reflects the infrequent need for reading log entries
directly from storage, as it primarily occurs when applying logs to the
state machine.

Inside Openraft, logs can now be read by acquiring a reader via
`RaftLogStorage.get_log_reader().await.xxx()`, rather than requiring
`RaftLogStorage` to implement the `RaftLogReader` trait.

Upgrade(non-breaking) tip:

Implementations are advised (though it is not mandatory) to remove the
`impl RaftLogReader for YourRaftLogStore` from their codebase.

- Fix: databendlabs#1118
@drmingdrmer drmingdrmer requested a review from schreter July 6, 2024 02:45
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.

Makes sense, thanks.

BTW, I assume reading the log is effectively only used during the restart when we find out the state machine lags behind the last committed log, right? Then, the local log entries up to local log end are read and applied, remaining entries are then received from the leader via regular replication, written to log and applied then (which is no different from regular online operation).

I.e., the entries on the leader received from the client are kept in memory until they are applied (or thrown out when it ceases to be a leader) and similarly, entries received from the leader on the follower are kept in memory until they are applied on the follower.

Thus, the log is effectively never read, except for the restart case, right?

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @drmingdrmer)

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.

Right.

For a Follower, the logs are read only when applying them. The RaftLogStorage implementation should keeps the logs in memory until applied. After that logs memory can be released.

For a Leader, logs are read when applying them and when replicating them to followers.
To support replication, the logs in memory should be kept for a longer time to maximize performance.

And logs are read when starting up.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @drmingdrmer)

@drmingdrmer drmingdrmer merged commit fa85822 into databendlabs:main Jul 7, 2024
32 checks passed
@drmingdrmer drmingdrmer deleted the 89-remove-log-reader-from-log-storage branch July 7, 2024 03:40
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.

Proposal: Remove RaftLogReader Trait Dependency from RaftLogStorage
2 participants