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(worker): Flush the stream writer on error (DGRAPH-2499) #6609

Merged
merged 6 commits into from
Nov 10, 2020

Conversation

jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Sep 30, 2020

The stream write blocks writes to badger and if we don't call writer.flush we will never unblock the writes


This change is Reviewable

Docs Preview: Dgraph Preview

return
}
// Flush the writer if it has not been flushed so that we can unblock the writes.
if err := writer.Flush(); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could add a cancel API on stream writer to fix this in a more elegant way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ajeetdsouza is working on resolving this.

Copy link
Contributor

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


worker/snapshot.go, line 82 at r1 (raw file):

Previously, jarifibrahim (Ibrahim Jarif) wrote…

We could add a cancel API on stream writer to fix this in a more elegant way.

Add a Cancel API as you suggested.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: Whenever you fix it, you can merge it.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @jarifibrahim and @vvbalaji-dgraph)

@jarifibrahim
Copy link
Contributor Author

Closing this PR since we don't need it anymore.

@jarifibrahim jarifibrahim deleted the ibrahim/snapshot-start branch October 15, 2020 12:52
@ajeetdsouza ajeetdsouza restored the ibrahim/snapshot-start branch November 2, 2020 10:21
@ajeetdsouza ajeetdsouza reopened this Nov 2, 2020
@ajeetdsouza ajeetdsouza self-assigned this Nov 6, 2020
@ajeetdsouza ajeetdsouza changed the title fix(worker): Flush the stream writer on error fix(worker): Flush the stream writer on error (DGRAPH-2499) Nov 6, 2020
@ajeetdsouza ajeetdsouza merged commit 7088c65 into master Nov 10, 2020
@jarifibrahim jarifibrahim deleted the ibrahim/snapshot-start branch March 24, 2021 10:01
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