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

During shutdown, generate snapshot before closing raft node. #5476

Merged
merged 2 commits into from
May 22, 2020

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented May 19, 2020

This will prevent trySnapshot from trying to send a range to the index
range channel before it's closed.

Fixes DGRAPH-1574


This change is Reviewable

This will prevent trySnapshot from trying to send a range to the index
range channel before it's closed.
@martinmr martinmr force-pushed the martinmr/close-range branch from 638d06c to acfbf8c Compare May 19, 2020 22:53
@martinmr martinmr changed the title Avoid panic when sending to indexRangeChan during shutdown. During shutdown, generate snapshot before closing raft node. May 19, 2020
Copy link
Contributor

@ashish-goswami ashish-goswami left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @manishrjain and @vvbalaji-dgraph)

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: Address the comments.

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @martinmr and @vvbalaji-dgraph)


dgraph/cmd/zero/run.go, line 296 at r1 (raw file):

		_ = httpListener.Close()
		// Try to generate a snapshot before the shutdown.
		st.node.trySnapshot(0)

We could do it after the node.closer and before the store closer, I think. Ask @ashish-goswami .

Ideally, we finish up whatever the node had, and take a snapshot after.

Copy link
Contributor Author

@martinmr martinmr 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 @ashish-goswami, @manishrjain, and @vvbalaji-dgraph)


dgraph/cmd/zero/run.go, line 296 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

We could do it after the node.closer and before the store closer, I think. Ask @ashish-goswami .

Ideally, we finish up whatever the node had, and take a snapshot after.

Done.

@martinmr martinmr merged commit 1dd2530 into master May 22, 2020
@martinmr martinmr deleted the martinmr/close-range branch May 22, 2020 18:59
martinmr added a commit that referenced this pull request May 22, 2020
This will prevent trySnapshot from trying to send a range to the index
range channel before it's closed.
martinmr added a commit that referenced this pull request May 22, 2020
This will prevent trySnapshot from trying to send a range to the index
range channel before it's closed.
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
…deinc#5476)

This will prevent trySnapshot from trying to send a range to the index
range channel before it's closed.
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