-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 copyRowsFunc hangs bug #611
Conversation
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.
Thank you for this observation! I believe you are correct in your observation.
However, I also think the solution here is mistaken. See my comment inline.
go/logic/migrator.go
Outdated
@@ -1135,6 +1140,9 @@ func (this *Migrator) iterateChunks() error { | |||
} | |||
// Enqueue copy operation; to be executed by executeWriteFuncs() | |||
this.copyRowsQueue <- copyRowsFunc | |||
if !hasFurtherRange { | |||
break | |||
} |
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.
Since this code is outside copyRowsFunc
, then it only skips enqueuing of new copyRowsFunc
functions, but still there could be multiple copyRowsFunc
instacnes inside this.copyRowsQueue
that haven't been evaluated yet.
This is why I believe this PR mitigates the potential for deadlock but does not actually solve it.
I think we should check hasFurtherRange
from within copyRowsFunc
to solve it completely.
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.
@shlomi-noach yes, you are right. I will make some modification.
2b78c3c
to
c7dff99
Compare
@shlomi-noach I have make some modification, will you pls. have a review. |
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.
this looks good. Let me send it to some testing.
Apologies for the time it took, I was sidetracked. On the flip side, this PR has been actively tested for some 50 days, so it's been well hammered. Merging! |
Related issue: #610
Description
When copyRowsFunc finishes copy rows, the status
hasFurtherRange
is kept across different iterations of copyRowsFunc, and whenhasFurtherRange
is false, never try to callCalculateNextIterationRangeEndValues
.When
hasFurtherRange
is false, the original table also might be locked and in this caseCalculateNextIterationRangeEndValues
will be hang forever.