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

Do not report snapshot.last_log_id to metrics until snapshot is finished building/installing #912

Closed
drmingdrmer opened this issue Oct 18, 2023 · 1 comment · Fixed by #913
Assignees

Comments

@drmingdrmer
Copy link
Member

          > One of the tests for snapshot install is failing from what looks like a race condition between the assert and the snapshot installation.

@zach-schoenberger
You are right. It is a race condition. The in-memory state about snapshot is already updated before actually installing snapshot to RaftStorage. Thus metrics may be ahead of the storage stage.

👉🏻1 Update in-memory state when installing snapshot:
https://github.com/datafuselabs/openraft/blob/496a0f87fb6f03b640f19987ad4ea549c75a6a8f/openraft/src/engine/handler/following_handler/mod.rs#L320

and then:
https://github.com/datafuselabs/openraft/blob/6098f5cf61b074c8fccecf722278183c3ee5cd27/openraft/src/engine/handler/snapshot_handler/mod.rs#L63

👉🏻2 Metrics is updated to the in-memory state. At this time point, the install-snapshot command may not have reached RaftStorage:
https://github.com/datafuselabs/openraft/blob/942ec78bc367665ce82516ad3458e479d895b0ea/openraft/src/core/raft_core.rs#L524

The solution should be split the snapshot state into two: the expected state when all io-commands are executed and the actual state of the RaftStorage. The storage state should be updated at this point, when notification about a snapshot being installed is received.:
https://github.com/datafuselabs/openraft/blob/942ec78bc367665ce82516ad3458e479d895b0ea/openraft/src/core/raft_core.rs#L1319-L1329

Let me fix this first

Originally posted by @drmingdrmer in #896 (comment)

@github-actions
Copy link

👋 Thanks for opening this issue!

Get help or engage by:

  • /help : to print help messages.
  • /assignme : to assign this issue to you.

drmingdrmer added a commit to drmingdrmer/openraft that referenced this issue Oct 18, 2023
…finished building/installing

Before this commit `RaftMetrics.snapshot` contains the last log id of a
snapshot that is **going** to install. Therefore there is chance the
metrics is updated but the store does not.

In this commit, `RaftMetrics.snapshot` will only be updated when a
snapshot is finished building or installing, by adding a new field
`snpashot` to `IOState` for tracking persisted snapshot meta data.

- Fix: databendlabs#912
@drmingdrmer drmingdrmer self-assigned this Oct 18, 2023
drmingdrmer added a commit to drmingdrmer/openraft that referenced this issue Nov 28, 2023
…finished building/installing

Before this commit `RaftMetrics.snapshot` contains the last log id of a
snapshot that is **going** to install. Therefore there is chance the
metrics is updated but the store does not.

In this commit, `RaftMetrics.snapshot` will only be updated when a
snapshot is finished building or installing, by adding a new field
`snpashot` to `IOState` for tracking persisted snapshot meta data.

- Fix: databendlabs#912
drmingdrmer added a commit to drmingdrmer/openraft that referenced this issue Nov 28, 2023
…finished building/installing

Before this commit `RaftMetrics.snapshot` contains the last log id of a
snapshot that is **going** to install. Therefore there is chance the
metrics is updated but the store does not.

In this commit, `RaftMetrics.snapshot` will only be updated when a
snapshot is finished building or installing, by adding a new field
`snpashot` to `IOState` for tracking persisted snapshot meta data.

- Fix: databendlabs#912
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant