Skip to content

Permit intra-L0 on ingested files in consistency check#67

Merged
ajkr merged 1 commit intocockroachdb:crl-release-6.2.1from
ajkr:relax-l0-consistency-check
Oct 15, 2019
Merged

Permit intra-L0 on ingested files in consistency check#67
ajkr merged 1 commit intocockroachdb:crl-release-6.2.1from
ajkr:relax-l0-consistency-check

Conversation

@ajkr
Copy link
Copy Markdown

@ajkr ajkr commented Oct 14, 2019

Previously, the consistency check was verifying files were ordered by
smallest seqnum after sorting them by largest seqnum. There was an
exception for ingested files since they could be assigned a seqnum in
the memtable's seqnum range if the memtable didn't contain overlapping
keys. However, this exception didn't account for ingested files
undergoing intra-L0 compaction, after which they can no longer be
identified as ingested. Now that we can no longer tell which files
should be excepted from this check, I just removed the whole thing.


This change is Reviewable

Previously, the consistency check was verifying files were ordered by
smallest seqnum after sorting them by largest seqnum. There was an
exception for ingested files since they could be assigned a seqnum in
the memtable's seqnum range if the memtable didn't contain overlapping
keys. However, this exception didn't account for ingested files
undergoing intra-L0 compaction, after which they can no longer be
identified as ingested. Now that we can no longer tell which files
should be excepted from this check, I just removed the whole thing.
@ajkr ajkr requested review from itsbilal and sumeerbhola October 14, 2019 22:27
Copy link
Copy Markdown

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

:lgtm: Nice that you were able to add a test for this condition. Does this need to be backported to 19.1 and earlier? I recall we backported enabling consistency checks.

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @itsbilal and @sumeerbhola)

Copy link
Copy Markdown

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @itsbilal)

Copy link
Copy Markdown

@itsbilal itsbilal 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! all files reviewed, all discussions resolved (waiting on @itsbilal)

Copy link
Copy Markdown
Author

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

Nice that you were able to add a test for this condition. Does this need to be backported to 19.1 and earlier? I recall we backported enabling consistency checks.

It should be backported to 19.2 as it supports running with ENABLE_ROCKSDB_ASSERTIONS=1, which enables these checks. For release builds we have not turned on these checks yet either on 19.2 or master -- that will require setting force_consistency_checks=true.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@ajkr ajkr merged commit 9d30412 into cockroachdb:crl-release-6.2.1 Oct 15, 2019
ajkr added a commit to ajkr/cockroach that referenced this pull request Oct 18, 2019
craig bot pushed a commit to cockroachdb/cockroach that referenced this pull request Oct 19, 2019
41728: c-deps: bump rocksdb for L0 file invariant fix r=ajkr a=ajkr

Picks up cockroachdb/rocksdb#67.

Release note: None

Co-authored-by: Andrew Kryczka <andrew.kryczka2@gmail.com>
ajkr added a commit to ajkr/cockroach that referenced this pull request Oct 19, 2019
arulajmani pushed a commit to arulajmani/cockroach that referenced this pull request Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants