Skip to content
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.

Sign up for GitHub

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

Jump to bottom

sql/schemachanger: job non-revertibility marking occurs one stage too late #76441

Closed
ajwerner opened this issue Feb 11, 2022 · 1 comment
Closed

sql/schemachanger: job non-revertibility marking occurs one stage too late #76441

ajwerner opened this issue Feb 11, 2022 · 1 comment
Assignees
@ajwerner
Labels
A-schema-changer-impl Related to the implementation of the new schema changer branch-master Failures and bugs on the master branch. branch-release-22.1 Used to mark GA and release blockers, technical advisories, and bugs for 22.1 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@ajwerner
Copy link
Contributor

ajwerner commented Feb 11, 2022 •
edited by cockroach-jira-scripts
Loading

Describe the problem
We need to mark the job as non-revertible as we finish the last stage which predeces the non-revertible phase, not the first stage of the non-revertible phase.

Jira issue: CRDB-13124
@ajwerner ajwerner added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. branch-master Failures and bugs on the master branch. release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. A-schema-changer-impl Related to the implementation of the new schema changer labels Feb 11, 2022
@blathers-crl blathers-crl bot added the T-sql-schema-deprecated Use T-sql-foundations instead label Feb 11, 2022
@ajwerner ajwerner self-assigned this Feb 11, 2022
@postamar postamar added GA-blocker and removed branch-master Failures and bugs on the master branch. release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. labels Mar 14, 2022
@ajwerner ajwerner mentioned this issue Mar 17, 2022
Merged
ajwerner added a commit to ajwerner/cockroach that referenced this issue Mar 17, 2022
@ajwerner
sql/schemachanger: set non-cancelable property on jobs correctly
0b926d9 
This PR sets the non-cancelability of jobs at the correct moment so that
there is not a phase where the job is cancelable but the state implies that
the job should not be reverted. It also propagates this state correctly to
descriptors so that it can be correctly synthesized during a restore.

Backport will address cockroachdb#76441.

Release note: None
ajwerner added a commit to ajwerner/cockroach that referenced this issue Mar 17, 2022
@ajwerner
sql/schemachanger: set non-cancelable property on jobs correctly
adf3520 
This PR sets the non-cancelability of jobs at the correct moment so that
there is not a phase where the job is cancelable but the state implies that
the job should not be reverted. It also propagates this state correctly to
descriptors so that it can be correctly synthesized during a restore.

Backport will address cockroachdb#76441.

Release note: None
@postamar postamar added branch-master Failures and bugs on the master branch. branch-release-22.1 Used to mark GA and release blockers, technical advisories, and bugs for 22.1 labels Mar 21, 2022
craig bot pushed a commit that referenced this issue Mar 23, 2022
@dt @ajwerner @otan
Merge #77588 #78022 #78163
62cdf19 
77588: kv/bulk: split/scatter after range says it is full r=dt a=dt

Previously we would split-and-scatter after adding "filling" a range so
that as we continued to fill, we would do so to a new range that was
first scattered to a new node. We did this once we had written 48MB to
one range, as determined by not hitting a split while sending that many
bytes during a single flush of the above buffer. 48MB was an OK default
when the default range size was 64MB, but has long since been far below
what we now consider full. This was a minor annoyance (causing merges)
but calling split-and-scatter so much more than is needed has recently
become a more severe issue now that ~every scatter call actually moves
a range and blocks on that snapshot.

Even when 48MB was a reasonable fraction of the 64MB default, this was
just a rough approximation for full that failed to account for the
actual range's configured size and only counted data flushed since the
last explicit Flush().

Instead, this change replaces this "split when i've sent enough" logic
with a new approach that lets the range itself reply to each SST with
its remaining capacity and end key. The caller can then use this to
determine if there is room to send another file to that range or if it
should consider it full, and split if/when another key that would be
added to that ranges span is added to the batch.

This significantly reduces how often ingestion stops to wait on scatter
of a split span with the recently updated scatter behavior, more than
doubling observed speeds of bulk ingestion.

Release justification: high impact fix to new/modified functionality.

Release note (performance improvement): Ranges are split and rebalanced during bulk ingestion only when they become full, reducing excessive splits and subsequent merging and associated rebalancing-related performance impact.


78022: sql/schemachanger: set non-cancelable property on jobs correctly r=ajwerner a=ajwerner

This PR sets the non-cancelability of jobs at the correct moment so that
there is not a phase where the job is cancelable but the state implies that
the job should not be reverted. It also propagates this state correctly to
descriptors so that it can be correctly synthesized during a restore.

Backport will address #76441.

Release note: None

78163: ttljob: fix range decoding bug r=rafiss a=otan

Previously, when a range was split by a prefix of a PRIMARY KEY, the TTL
job would fail. This PR rectifies that.

Resolves #78162
Release note: None

Co-authored-by: David Taylor <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>
ajwerner added a commit to ajwerner/cockroach that referenced this issue Mar 24, 2022
@ajwerner
sql/schemachanger: set non-cancelable property on jobs correctly
f2c8f70 
This PR sets the non-cancelability of jobs at the correct moment so that
there is not a phase where the job is cancelable but the state implies that
the job should not be reverted. It also propagates this state correctly to
descriptors so that it can be correctly synthesized during a restore.

Backport will address cockroachdb#76441.

Release note: None
ajwerner added a commit to ajwerner/cockroach that referenced this issue Mar 25, 2022
@ajwerner
sql/schemachanger: set non-cancelable property on jobs correctly
cd3af30 
This PR sets the non-cancelability of jobs at the correct moment so that
there is not a phase where the job is cancelable but the state implies that
the job should not be reverted. It also propagates this state correctly to
descriptors so that it can be correctly synthesized during a restore.

Backport will address cockroachdb#76441.

Release note: None
craig bot pushed a commit that referenced this issue Mar 25, 2022
@ajwerner
Merge #78022
531eee8 
78022: sql/schemachanger: set non-cancelable property on jobs correctly r=ajwerner a=ajwerner

This PR sets the non-cancelability of jobs at the correct moment so that
there is not a phase where the job is cancelable but the state implies that
the job should not be reverted. It also propagates this state correctly to
descriptors so that it can be correctly synthesized during a restore.

Backport will address #76441.

Release note: None

Co-authored-by: Andrew Werner <[email protected]>
blathers-crl bot pushed a commit that referenced this issue Mar 27, 2022
@ajwerner
sql/schemachanger: set non-cancelable property on jobs correctly
6ff05ac 
This PR sets the non-cancelability of jobs at the correct moment so that
there is not a phase where the job is cancelable but the state implies that
the job should not be reverted. It also propagates this state correctly to
descriptors so that it can be correctly synthesized during a restore.

Backport will address #76441.

Release note: None
@blathers-crl blathers-crl bot mentioned this issue Mar 27, 2022
Merged
@ajwerner
Copy link
Contributor Author

Fixed by #78577.

@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ajwerner ajwerner

Labels
A-schema-changer-impl Related to the implementation of the new schema changer branch-master Failures and bugs on the master branch. branch-release-22.1 Used to mark GA and release blockers, technical advisories, and bugs for 22.1 C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@ajwerner @postamar