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

release-22.1: spanconfig,kvserver: teach the GC job to respect tenant PTS records #78389

Merged
merged 2 commits into from
Mar 24, 2022
Merged

release-22.1: spanconfig,kvserver: teach the GC job to respect tenant PTS records #78389

merged 2 commits into from
Mar 24, 2022

Conversation

blathers-crl[bot]
Copy link

@blathers-crl blathers-crl bot commented Mar 23, 2022

Backport 2/2 commits from #77726 on behalf of @adityamaru.

/cc @cockroachdb/release

This change teaches the GC job responsible for ClearRange'ing a tenants
data to respect protected timestamp records that are protecting a tenants
data. Specifically, once the tenants row data has expired, the GC job now
checks for protected timestamp records that have been written by the system
tenant targetting that particular tenant, or targetting its entire keyspace
i.e. all secondary tenants. If the record is protecting a time less than the
drop time of the tenant then the GC will be delayed.

This change also consolidates all tenant GC related tests into one file. The only
new test is TestGCTenantJobWaitsForProtectedTimestamps.

Release note (enterprise change): Tenant GC job will now wait for protected
timestamp records targetting the tenant, and with a protect time less than
the tenant's drop time.

Release justification: high-priority changes in existing functionality

Fixes: #77239

Release justification:

@adityamaru
gcjob: rename isProtected to deprecatedIsProtected
5b6836c 
Release note: None

Release justification: fixes for high-priority bugs in existing functionality
@adityamaru
spanconfig,kvserver: teach the GC job to respect tenant PTS records
9dc9536 
This change teaches the GC job responsible for ClearRange'ing a tenants
data to respect protected timestamp records written by the system tenant
covering the secondary tenant's keyspace. Specifically, once the tenants
row data has expired, the GC job now checks for protected timestamp records
that have been written by the system tenant targetting that particular
tenant, or targetting its entire keyspace i.e. all secondary tenants.
If the record is protecting a time less than the drop time of the tenant
then the GC will be delayed.

This is important because a system tenant may be backing up the secondary tenant
or all secondary tenants while the secondary tenant has been dropped. We don't want
the GC job to ClearRange the tenant's data until the backup that wrote the PTS record
on the tenant's keyspace has completed (backed up the tenant data) and released the record.

This change also consolidates all tenant GC related tests into one file. The only
new test is `TestGCTenantJobWaitsForProtectedTimestamps`.

Release note (enterprise change): Tenant GC job will now wait for protected
timestamp records targetting the tenant, and with a protect time less than
the tenant's drop time.

Release justification: high-priority changes in existing functionality
@blathers-crl blathers-crl bot requested review from a team as code owners March 23, 2022 23:39
@blathers-crl blathers-crl bot requested a review from a team March 23, 2022 23:39
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-22.1-77726 branch from 16277cc to 9dc9536 Compare March 23, 2022 23:39
@blathers-crl
Copy link
Author

blathers-crl bot commented Mar 23, 2022 •
edited by adityamaru
Loading

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@blathers-crl blathers-crl bot added blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot. labels Mar 23, 2022
@cockroach-teamcity
Copy link
Member

This change is Reviewable

ajwerner
ajwerner approved these changes Mar 24, 2022
View reviewed changes
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru and @arulajmani)

@adityamaru adityamaru merged commit face5a8 into release-22.1 Mar 24, 2022
@adityamaru adityamaru deleted the blathers/backport-release-22.1-77726 branch March 24, 2022 05:34
@cockroach-teamcity cockroach-teamcity mentioned this pull request Mar 24, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajwerner ajwerner ajwerner approved these changes

@adityamaru adityamaru Awaiting requested review from adityamaru

@arulajmani arulajmani Awaiting requested review from arulajmani

Assignees

@adityamaru adityamaru

Labels
blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cockroach-teamcity @ajwerner @adityamaru