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.

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

internal/metamorphic: write range keys #1503

Merged
merged 1 commit into from
Feb 14, 2022

Conversation

jbowens
Copy link
Collaborator

@jbowens jbowens commented Feb 9, 2022

Update the metamorphic tests to write range keys. This change does not yet add
reading of range keys to the metamorphic tests. Adding support for reading is
more complicated, broad change that will be added in a subsequent PR.

@jbowens jbowens requested a review from nicktrav February 9, 2022 19:56
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jbowens jbowens force-pushed the meta-write-rangekeys branch from 9fd5746 to 4ea9f4d Compare February 9, 2022 20:34
Copy link
Contributor

@nicktrav nicktrav left a comment

Choose a reason for hiding this comment

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

Neat! :lgtm:

Reviewed 5 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @itsbilal, @jbowens, and @sumeerbhola)


-- commits, line 5 at r1:
typo: s/is/is a/


internal/metamorphic/generator.go, line 925 at r1 (raw file):

}

func (g *generator) writerRangeKeyUnset() {

Wondering if we want some way of tracking intervals that have already been set, so that there's some increased probability of unsets hitting ranges that are already set. Same thing applies for dels tracking ranges that are already set / unset.

Perhaps a TODO, if you think it's useful / indicative of real workloads?

Copy link
Collaborator

@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.

:lgtm:

Reviewed 4 of 5 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @itsbilal and @jbowens)


internal/metamorphic/generator.go, line 173 at r2 (raw file):

}

func (g *generator) randSuffix(incMaxProb float64) []byte {

can you add a method comment.


internal/metamorphic/generator.go, line 897 at r2 (raw file):

	}

	start := g.randPrefixToWrite(0.001)

Can the following few lines be abstracted into a function that generates the [start, end) pair. Seems to be the same code repeated in 3 places.

Update the metamorphic tests to write range keys. This change does not yet add
reading of range keys to the metamorphic tests. Adding support for reading is a
more complicated, broad change that will be added in a subsequent PR.
@jbowens jbowens force-pushed the meta-write-rangekeys branch from 4ea9f4d to 99861f6 Compare February 10, 2022 21:24
Copy link
Collaborator Author

@jbowens jbowens left a comment

Choose a reason for hiding this comment

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

TFTRs!

Reviewable status: 4 of 5 files reviewed, 3 unresolved discussions (waiting on @itsbilal, @nicktrav, and @sumeerbhola)


internal/metamorphic/generator.go, line 925 at r1 (raw file):

Previously, nicktrav (Nick Travers) wrote…

Wondering if we want some way of tracking intervals that have already been set, so that there's some increased probability of unsets hitting ranges that are already set. Same thing applies for dels tracking ranges that are already set / unset.

Perhaps a TODO, if you think it's useful / indicative of real workloads?

Good call, added a TODO.


internal/metamorphic/generator.go, line 173 at r2 (raw file):

Previously, sumeerbhola wrote…

can you add a method comment.

Done.


internal/metamorphic/generator.go, line 897 at r2 (raw file):

Previously, sumeerbhola wrote…

Can the following few lines be abstracted into a function that generates the [start, end) pair. Seems to be the same code repeated in 3 places.

Done.

Copy link
Collaborator

@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.

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @itsbilal, @nicktrav, and @sumeerbhola)

@jbowens jbowens merged commit b1af263 into cockroachdb:master Feb 14, 2022
@jbowens jbowens deleted the meta-write-rangekeys branch February 14, 2022 12:38
jbowens added a commit to jbowens/pebble that referenced this pull request Feb 14, 2022
Now that range keys are gated behind a format major version and the metamorphic
tests write range keys, we must use at least FormatRangeKeys in the metamorphic
tests.

Fix merge skew from cockroachdb#1497 and cockroachdb#1503.
jbowens added a commit that referenced this pull request Feb 14, 2022
Now that range keys are gated behind a format major version and the metamorphic
tests write range keys, we must use at least FormatRangeKeys in the metamorphic
tests.

Fix merge skew from #1497 and #1503.
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