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

fix(raftwal): take snapshot after restore #7719

Merged
merged 4 commits into from
Apr 13, 2021
Merged

Conversation

NamanJain8
Copy link
Contributor

@NamanJain8 NamanJain8 commented Apr 13, 2021

Fixes DGRAPH-3202

We should propose a snapshot immediately after the restore to prevent the restore from being replayed.
Earlier we were trying to that too. Snapshot happens after the restore but that does not actually include the restore proposal as it has not been applied yet (n.Applied.Done() is not yet called for that proposal).
Now, we will first wait for the raft entries to be marked as done and then try to propose the snapshot.


This change is Reviewable

@NamanJain8 NamanJain8 changed the title fix(raftwal): do best effort for snapshot after restore fix(raftwal): take snapshot after restore Apr 13, 2021
Copy link
Contributor

@ahsanbarkati ahsanbarkati left a comment

Choose a reason for hiding this comment

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

Looks good to me. I was wondering if the snapshot should be proposed only by the leader, but seems like this is also fine considering only one proposal will be applied. And all other proposals after it will be no-op.

worker/draft.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jarifibrahim jarifibrahim left a comment

Choose a reason for hiding this comment

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

Looks good to me. @NamanJain8 we could verify the snapshot TS in the online restore test.

You can query the /state endpoint and check if the snapshot Ts was different before the restore request was sent.
You don't need a new test. Just use one of the existing tests.

Copy link
Contributor

@ahsanbarkati ahsanbarkati left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@ahsanbarkati ahsanbarkati 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: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @jarifibrahim, @manishrjain, and @vvbalaji-dgraph)

Looks good to me

@NamanJain8 NamanJain8 merged commit 72cebd1 into master Apr 13, 2021
@NamanJain8 NamanJain8 deleted the naman/restore-replay branch April 13, 2021 18:48
NamanJain8 added a commit that referenced this pull request Apr 26, 2021
We should propose a snapshot immediately after the restore to prevent the restore from being replayed.
Earlier we were trying to do that too. Snapshot happens after the restore but that does not actually include the restore proposal as it has not been applied yet (n.Applied.Done() is not yet called for that proposal).
Now, we will first wait for the raft entry to be marked as done and then try to propose the snapshot.

(cherry picked from commit 72cebd1)
NamanJain8 added a commit that referenced this pull request Apr 26, 2021
We should propose a snapshot immediately after the restore to prevent the restore from being replayed.
Earlier we were trying to do that too. Snapshot happens after the restore but that does not actually include the restore proposal as it has not been applied yet (n.Applied.Done() is not yet called for that proposal).
Now, we will first wait for the raft entry to be marked as done and then try to propose the snapshot.

(cherry picked from commit 72cebd1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants