-
Notifications
You must be signed in to change notification settings - Fork 219
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
Block resuming a partition merge schema change #4936
base: 8.0
Are you sure you want to change the base?
Conversation
…e sharing the destination newdb, which involves more complicated code changes and will be adressed in 8.1. Signed-off-by: Dorin Hogea <[email protected]>
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.
Coding style check: Error. ⚠.
Smoke testing: Error ⚠.
Cbuild submission: Success ✓.
Regression testing: 6/564 tests failed ⚠.
The first 10 failing tests are:
fdb_push [setup failure]
mismatch_class_remsql_fdbpushredirect_generated [setup failure]
phys_rep_perf
sc_timepart
lostwrite
truncatesc_offline_generated
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.
.
} else if (s->finalize == 0 && s->kind != SC_ALTERTABLE_PENDING) { | ||
s->sc_next = sc_resuming; | ||
sc_resuming = s; | ||
} |
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.
My understanding of this is that you want to process and potentially resume schema changes on all of the shards via verify_sc_resumed_for_shard
instead of via start_schema_change
, and that's why you moved the conditions around. Is that right?
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.
Correct, otherwise I would start a schema change for a shard ((because it was already in progress on the previous master), before we check that this is a merge that needs to be blocked.
} else if (s->finalize == 0 && s->kind != SC_ALTERTABLE_PENDING) { | ||
s->sc_next = sc_resuming; | ||
sc_resuming = s; | ||
} | ||
} | ||
} | ||
} |
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.
Are there any existing tests for resuming partitioned schema changes?
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.
A few tests swing the master during schema changes, sc_resume is dedicated for that
Resuming would require sharing the destination newdb, which involves more complicated code changes and will be adressed in 8.1.