Skip to content

refactor: extract can_safely_apply() and improve IOState documentation#1713

Closed
lichuang wants to merge 1 commit intodatabendlabs:mainfrom
lichuang:optimize-can_safely_apply
Closed

refactor: extract can_safely_apply() and improve IOState documentation#1713
lichuang wants to merge 1 commit intodatabendlabs:mainfrom
lichuang:optimize-can_safely_apply

Conversation

@lichuang
Copy link
Copy Markdown
Contributor

@lichuang lichuang commented Apr 12, 2026

Refactor the "leader safety invariant" check in Engine into a dedicated IOState::can_safely_apply() method. This makes the safety condition more explicit and testable.

Add comprehensive documentation explaining:

  • The Engine/Runtime separation architecture (logical vs physical state)
  • The three-stage I/O progress model (accepted/submitted/flushed)
  • The monotonicity invariant: accepted >= submitted >= flushed
  • The leader safety invariant for safe log application

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

Refactor the "leader safety invariant" check in Engine into a dedicated
IOState::can_safely_apply() method. This makes the safety condition more
explicit and testable.

Add comprehensive documentation explaining:
- The Engine/Runtime separation architecture (logical vs physical state)
- The three-stage I/O progress model (accepted/submitted/flushed)
- The monotonicity invariant: accepted >= submitted >= flushed
- The leader safety invariant for safe log application
@drmingdrmer
Copy link
Copy Markdown
Member

Thanks for the refactoring — extracting can_safely_apply() is a clean improvement in principle, but I have some concerns about both the abstraction choice and the documentation accuracy.


1. Prefer inline condition over can_safely_apply()

I'd prefer keeping the explicit comparison inline rather than extracting it into can_safely_apply(). The method name is an abstract description that doesn't reveal the actual condition being checked — it could be interpreted as checking several possible safety properties.

The real concern here is narrow and specific: there must be no queued command that could override submitted log entries. The inline comparison submitted.vote == accepted.vote makes this reasoning visible at the call site — same leader means no truncation can be queued. Wrapping it in can_safely_apply() hides that reasoning behind a name that is too broad.


2. "flushed → safe to apply" is incorrect

In the IOProgress three-stage diagram:

flushed  ────>  ...  Durable on disk; safe to apply

Being flushed is not a condition for applying a log entry. A log entry only needs to be submitted to be applied — the state machine reads from the storage layer, which can serve submitted entries before they are flushed to disk.

The existing doc in io_state.rs already states this correctly:

Note: apply.accepted does not require log.flushed. A log only needs to be submitted (not flushed) to be applied, since RaftLogStorage can read submitted entries.


3. vote field doc: "vote.is_committed() == true → established leader" is inaccurate

The added comment:

Monotonically increasing in term. When vote.is_committed() == true, this node is an established leader (granted by a quorum).

Two issues:

  • Vote is monotonically increasing in the vote's total order, not just term. A vote with the same term but a higher node ID also advances.
  • vote.is_committed() means this node has accepted a committed vote — it could be a follower that accepted a leader's vote via save-vote. It does not mean "this node is the leader."

4. Minor: RaftState vs IOState diagram could confuse

The diagram presents RaftState as the Engine's logical view and IOState as the Runtime's physical view. But IOState is a field inside RaftState (pub(crate) io_state: Valid<IOState<C>>). It would be clearer to say the other fields of RaftState represent the logical view, while io_state tracks physical I/O progress.

@drmingdrmer
Copy link
Copy Markdown
Member

Thanks for the refactoring — extracting can_safely_apply() is a clean improvement in principle, but I have some concerns about both the abstraction choice and the documentation accuracy.

I'm closing this PR due to the above considerations

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