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

clusterversion,*: remove VersionContainsEstimatesCounter #57155

Conversation

irfansharif
Copy link
Contributor

@irfansharif irfansharif commented Nov 26, 2020

This, and all surrounding migration code and tests, are now safe to
remove. It mostly served as documentation, which we've moved to the
field itself. Part of #47447. Fixes #56401.

(While here, Let's also tell git that versionkey_string.go is a
generated file.)

Release note: None


First commit is from #57154.

@irfansharif irfansharif requested a review from tbg November 26, 2020 02:00
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, 13 of 13 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif)


pkg/kv/kvserver/replica_proposal.go, line 795 at r2 (raw file):

		res.Replicated.Delta = ms.ToStatsDelta()

		// Encode that this command (and any that follow) uses regular

Add something like this

// This is the result of a migration. See the field for more details.

@irfansharif irfansharif force-pushed the 201125.del-VersionContainsEstimatesCounter branch from eae9904 to 0115b79 Compare November 27, 2020 21:12
And for deleting old ones. This stuff is endlessly confusing.

Release note: None
@irfansharif irfansharif force-pushed the 201125.del-VersionContainsEstimatesCounter branch from 0115b79 to 292c49e Compare November 27, 2020 22:02
Copy link
Contributor Author

@irfansharif irfansharif left a comment

Choose a reason for hiding this comment

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

bors r+

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @tbg)


pkg/kv/kvserver/replica_proposal.go, line 795 at r2 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Add something like this

// This is the result of a migration. See the field for more details.

Done.

Jump to symbol for a given version tag will now group together the
intent of the version itself (instead of expecting readers to know to
grep for the same version in the block below).

Release note: None
@irfansharif
Copy link
Contributor Author

bors r-

@craig
Copy link
Contributor

craig bot commented Nov 27, 2020

Canceled.

@irfansharif
Copy link
Contributor Author

bors r+

This, and all surrounding migration code and tests, are now safe to
remove. It mostly served as documentation, which we've moved to the
field itself. Part of cockroachdb#47447. Fixes cockroachdb#56401.

(While here, Let's also tell git that `versionkey_string.go` is a
generated file.)

Release note: None
@irfansharif irfansharif force-pushed the 201125.del-VersionContainsEstimatesCounter branch from 292c49e to 8107022 Compare November 27, 2020 22:23
@craig
Copy link
Contributor

craig bot commented Nov 27, 2020

Canceled.

@irfansharif
Copy link
Contributor Author

bors r+

craig bot pushed a commit that referenced this pull request Nov 27, 2020
57118: migration,server: plumb in initial version to the migration manager r=irfansharif a=irfansharif

It'll let us return early, and makes the manager "more functional" in its
behavior. 

We also promote the clusterversion type to a first-class citizen in the
external facing API for pkg/migration. This package concerns itself with
migrations between cluster versions and we should have our API reflect as much.

The proto changes are safe, we haven't had a major release with the previous
proto definitions.

Release note: None

57155: clusterversion,*: remove VersionContainsEstimatesCounter r=irfansharif a=irfansharif

This, and all surrounding migration code and tests, are now safe to
remove. It mostly served as documentation, which we've moved to the
field itself. Part of #47447. Fixes #56401.

(While here, Let's also tell git that `versionkey_string.go` is a
generated file.)

Release note: None

---

First commit is from #57154.

57156: sql,clusterversion: remove VersionAuthLocalAndTrustRejectMethods r=irfansharif a=irfansharif

It's an old cluster version, introduced in the 19.2 release cycle. It's
now safe to remove. Part of #47447. Fixes #56398.


Release note: None

---

See only last commit. First two are from #57154 and #57155 respectively.

Co-authored-by: irfan sharif <[email protected]>
@craig
Copy link
Contributor

craig bot commented Nov 27, 2020

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Nov 28, 2020

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Nov 28, 2020

Build succeeded:

@craig craig bot merged commit effee63 into cockroachdb:master Nov 28, 2020
@irfansharif irfansharif deleted the 201125.del-VersionContainsEstimatesCounter branch November 28, 2020 01:35
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.

kvserver: clean up VersionContainsEstimatesCounter migration from 20.1
3 participants