-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Atomic Copy: Fix panics when the copy phase starts in some clusters #17717
Atomic Copy: Fix panics when the copy phase starts in some clusters #17717
Conversation
…s due to bugs Signed-off-by: Rohit Nayak <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17717 +/- ##
==========================================
+ Coverage 67.94% 67.96% +0.01%
==========================================
Files 1586 1586
Lines 255173 255229 +56
==========================================
+ Hits 173389 173476 +87
+ Misses 81784 81753 -31 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Rohit Nayak <[email protected]>
log.Warningf("appendFromRow: wrong number of lengths for table %s: got %d lengths for %d fields", | ||
tp.TablePlanBuilder.name, len(row.Lengths), len(tp.Fields)) | ||
for i, fld := range tp.Fields { | ||
log.Warningf("appendFromRow: field:: %d:%s/%s", i, fld.Table, fld.Name) |
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.
Nit, but table.column would be more clear
I tested this code with the atomic move that failed for me with
|
Signed-off-by: Rohit Nayak <[email protected]>
6a76093
to
c2cfa0e
Compare
We isolated the issue to being related to concurrent inserts (the |
…17717) Signed-off-by: Rohit Nayak <[email protected]>
…some clusters (#17717) (#17748) Signed-off-by: Rohit Nayak <[email protected]> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
…some clusters (#17717) (#17746) Signed-off-by: Rohit Nayak <[email protected]> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com> Co-authored-by: Rohit Nayak <[email protected]>
…some clusters (#17717) (#17747) Signed-off-by: Rohit Nayak <[email protected]> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com> Co-authored-by: Rohit Nayak <[email protected]>
Description
Note that this doesn't fix the underlying issue that was reported but just fixes the panic, adds debug logging and returns an error so that vttablet doesn't crash.
It does fix a bug in a list initialization, used only for logging in atomicCopy mode, that was resulting in a confusing log message.
Unable to repro the underlying issue, so we will create a new issue with details when we get more info on it.
Backporting to all supported versions to prevent the panic.
Related Issue(s)
Fixes #17716
Checklist
Deployment Notes