-
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
track operations and cancel when needed #4916
Conversation
ad016c9
to
398c700
Compare
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, all discussions resolved (waiting on @manishrjain)
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 3 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @arijitAD, @mangalaman93, and @manishrjain)
worker/draft.go, line 152 at r1 (raw file):
glog.Infof("Operation complete with id: %d", id)
completed
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: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @arijitAD, @ashish-goswami, and @manishrjain)
worker/draft.go, line 152 at r1 (raw file):
Previously, ashish-goswami (Ashish Goswami) wrote…
glog.Infof("Operation complete with id: %d", id)
completed
Done.
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 2 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @arijitAD, @ashish-goswami, and @mangalaman93)
worker/draft.go, line 85 at r2 (raw file):
opRollUp = iota + 1 opSnapshot opBGIndex
opIndexing
worker/draft.go, line 90 at r2 (raw file):
// startRollUp will check whether rollup is already going on. If not, // it will return a context that can be used to cancel the rollup if needed. func (n *node) startRollUp() (context.Context, error) {
Avoid a special function for just one task.
worker/draft.go, line 105 at r2 (raw file):
// If rollup is going on, we cancel and wait for rollup to complete // before we return. If the same task is already going, we return error. func (n *node) startTask(id int) error {
Can return context.
worker/draft.go, line 110 at r2 (raw file):
// For bgindex, we may not call stopTask. if !schema.State().IndexingInProgress() {
I'd avoid adding this concept here. This should only track and act on the op.
worker/draft.go, line 151 at r2 (raw file):
// Signal that task is completed or cancelled. op.done <- struct{}{}
close(op.done)
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: 1 of 3 files reviewed, 6 unresolved discussions (waiting on @arijitAD, @ashish-goswami, @mangalaman93, and @manishrjain)
worker/draft.go, line 85 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
opIndexing
Done.
worker/draft.go, line 105 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Can return context.
Done.
worker/draft.go, line 151 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
close(op.done)
Done.
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 2 of 2 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @arijitAD, @ashish-goswami, and @mangalaman93)
worker/draft.go, line 106 at r4 (raw file):
<-rop.done glog.Info("Rollup cancelled.") } else if _, has := n.ops[id]; has {
if len(n.ops) > 0 ... error out.
worker/mutation.go, line 142 at r4 (raw file):
// Ensure that rollup is not running. tmpCtx, err := gr.Node.startTask(opIndexing)
don't call it tmpcontext.
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: all files reviewed, 6 unresolved discussions (waiting on @arijitAD, @ashish-goswami, @golangcibot, and @manishrjain)
worker/draft.go, line 90 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Avoid a special function for just one task.
Done.
worker/draft.go, line 110 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
I'd avoid adding this concept here. This should only track and act on the op.
Done.
worker/draft.go, line 106 at r4 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
if len(n.ops) > 0 ... error out.
Done.
worker/mutation.go, line 145 at r3 (raw file):
Previously, golangcibot (Bot from GolangCI) wrote…
if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (from
golint
)
Done.
worker/mutation.go, line 142 at r4 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
don't call it tmpcontext.
Done.
8d1bf88
to
0b869ed
Compare
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 2 of 2 files at r5.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @arijitAD, @ashish-goswami, @golangcibot, and @manishrjain)
we now track operations in the system such as rollup, indexing and snapshots. We cancel rollup whenever indexing or snapshot needs to be taken. We also do not allow multiple operations at the same time.
This change is