-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Alpha: Gracefully shutdown ludicrous mode #5561
Conversation
When live loader in running on an Alpha in ludricrous mode, alpha will crash with a segmentation fault error because badger was closed before ludricrous mode goroutines were stopped. This PR fixed that issue.
There was a problem hiding this 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 2 files reviewed, 1 unresolved discussion (waiting on @jarifibrahim, @manishrjain, and @vvbalaji-dgraph)
worker/executor.go, line 68 at r1 (raw file):
return default: }
We should avoid using it in critical path, might hamper performance significantly.
There was a problem hiding this 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 2 files reviewed, 1 unresolved discussion (waiting on @ashish-goswami, @manishrjain, and @vvbalaji-dgraph)
worker/executor.go, line 68 at r1 (raw file):
Previously, ashish-goswami (Ashish Goswami) wrote…
We should avoid using it in critical path, might hamper performance significantly.
Without this return statement, Alpha took around 1 minute to close in my tests. It could take more than 1 minute if palyload.edges
is huge.
There was a problem hiding this 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 2 files reviewed, 1 unresolved discussion (waiting on @jarifibrahim, @manishrjain, and @vvbalaji-dgraph)
worker/executor.go, line 68 at r1 (raw file):
Previously, jarifibrahim (Ibrahim Jarif) wrote…
Without this return statement, Alpha took around 1 minute to close in my tests. It could take more than 1 minute if
palyload.edges
is huge.
How big is payload.edges in your case? If its too big we can do it every 10K or 100K edge.
There was a problem hiding this 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 2 files reviewed, 1 unresolved discussion (waiting on @ashish-goswami, @manishrjain, and @vvbalaji-dgraph)
worker/executor.go, line 68 at r1 (raw file):
Previously, ashish-goswami (Ashish Goswami) wrote…
How big is payload.edges in your case? If its too big we can do it every 10K or 100K edge.
I see a maximum of 1000 edges but I think that wasn't the bottleneck. I've moved the select statement to check at the channel range loop. It takes 7 seconds now for Alpha to close after hitting ctrl+c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dismissed @ashish-goswami from a discussion.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @manishrjain and @vvbalaji-dgraph)
worker/executor.go, line 68 at r1 (raw file):
Previously, jarifibrahim (Ibrahim Jarif) wrote…
I see a maximum of 1000 edges but I think that wasn't the bottleneck. I've moved the select statement to check at the channel range loop. It takes 7 seconds now for Alpha to close after hitting ctrl+c
I had a chat we @ashish-goswami and we decided that this change is not needed right now.
There was a problem hiding this 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, all discussions resolved (waiting on @manishrjain and @vvbalaji-dgraph)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @manishrjain and @vvbalaji-dgraph)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @manishrjain and @vvbalaji-dgraph)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's separately clean up how closing happens.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @manishrjain and @vvbalaji-dgraph)
Fixes - DGRAPH-1353 When live loader in running on an Alpha in ludicrous mode, alpha will crash with a segmentation fault error because badger was closed before ludicrous mode goroutines were stopped. This PR fixed that issue. The crash can be reproduced by ``` 1. dgraph zero --ludicrous_mode 2. dgraph alpha --ludicrous_mode --enable_sentry=false 3. dgraph live -f $GOPATH/src/github.com/dgraph-io/benchmarks/data/21million.rdf.gz \ -s $GOPATH/src/github.com/dgraph-io/benchmarks/data/21million.schema \ -z localhost:5080 -a localhost:9080 --ludicrous_mode Hit <CTRL+C> while alpha is receiving data from live loader. ``` (cherry picked from commit e326b9e)
Fixes - DGRAPH-1353 When live loader in running on an Alpha in ludicrous mode, alpha will crash with a segmentation fault error because badger was closed before ludicrous mode goroutines were stopped. This PR fixed that issue. The crash can be reproduced by ``` 1. dgraph zero --ludicrous_mode 2. dgraph alpha --ludicrous_mode --enable_sentry=false 3. dgraph live -f $GOPATH/src/github.com/dgraph-io/benchmarks/data/21million.rdf.gz \ -s $GOPATH/src/github.com/dgraph-io/benchmarks/data/21million.schema \ -z localhost:5080 -a localhost:9080 --ludicrous_mode Hit <CTRL+C> while alpha is receiving data from live loader. ``` (cherry picked from commit e326b9e)
Fixes - DGRAPH-1353 When live loader in running on an Alpha in ludicrous mode, alpha will crash with a segmentation fault error because badger was closed before ludicrous mode goroutines were stopped. This PR fixed that issue. The crash can be reproduced by ``` 1. dgraph zero --ludicrous_mode 2. dgraph alpha --ludicrous_mode --enable_sentry=false 3. dgraph live -f $GOPATH/src/github.com/dgraph-io/benchmarks/data/21million.rdf.gz \ -s $GOPATH/src/github.com/dgraph-io/benchmarks/data/21million.schema \ -z localhost:5080 -a localhost:9080 --ludicrous_mode Hit <CTRL+C> while alpha is receiving data from live loader. ```
Fixes - DGRAPH-1353
When live loader in running on an Alpha in ludicrous mode, alpha will
crash with a segmentation fault error because badger was closed before
ludicrous mode goroutines were stopped. This PR fixed that issue.
The crash can be reproduced by
This change is