Skip to content

storage: Remove the last vestiges of non-proposer-evaluated-KV#15228

Merged
bdarnell merged 2 commits intocockroachdb:masterfrom
bdarnell:below-raft-protos
Apr 23, 2017
Merged

storage: Remove the last vestiges of non-proposer-evaluated-KV#15228
bdarnell merged 2 commits intocockroachdb:masterfrom
bdarnell:below-raft-protos

Conversation

@bdarnell
Copy link
Copy Markdown
Contributor

This allows us to remove nearly all protobufs from the "below raft"
list of types requiring strict compatibility.

@bdarnell bdarnell added this to the 1.0 milestone Apr 20, 2017
@bdarnell bdarnell requested a review from tamird April 20, 2017 20:23
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@petermattis
Copy link
Copy Markdown
Collaborator

:lgtm:


Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


pkg/storage/replica.go, line 3655 at r1 (raw file):

	{
		if raftCmd.ReplicatedEvalResult == nil && forcedErr == nil {
			panic("non-proposer-evaluated command found in raft log")

I wonder if this should be more verbose and mention what action the user should take or point to this week's beta release notes.


Comments from Reviewable

@bdarnell bdarnell force-pushed the below-raft-protos branch from 358cc9c to bd03a7e Compare April 20, 2017 20:38
@bdarnell
Copy link
Copy Markdown
Contributor Author

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.


pkg/storage/replica.go, line 3655 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I wonder if this should be more verbose and mention what action the user should take or point to this week's beta release notes.

Done.


Comments from Reviewable

@tamird
Copy link
Copy Markdown
Contributor

tamird commented Apr 20, 2017

Reviewed 3 of 3 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


pkg/storage/track_raft_protos.go, line 46 at r1 (raw file):

	whitelist := []string{
		// Some raft operations trigger gossip, but we don't require
		// strict consitency there.

"consistency"


Comments from Reviewable

@bdarnell bdarnell force-pushed the below-raft-protos branch from bd03a7e to c400022 Compare April 20, 2017 20:41
@tamird
Copy link
Copy Markdown
Contributor

tamird commented Apr 21, 2017

:lgtm:


Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@tamird
Copy link
Copy Markdown
Contributor

tamird commented Apr 21, 2017

I think you caught some build skew related to my rocksdb patch here, can you rebase for good measure?

@bdarnell bdarnell force-pushed the below-raft-protos branch from c400022 to ae805c4 Compare April 21, 2017 18:09
@bdarnell
Copy link
Copy Markdown
Contributor Author

And now it's (correctly) failing the reference binary acceptance tests. It's time to bump up the version used there.

bdarnell added a commit to cockroachdb/postgres-test that referenced this pull request Apr 22, 2017
Remove test data for cockroachdb/cockroach#7429 because we no longer
support upgrading from that version.

See cockroachdb/cockroach#15228
bdarnell added a commit to cockroachdb/postgres-test that referenced this pull request Apr 22, 2017
Remove test data for cockroachdb/cockroach#7429 because we no longer
support upgrading from that version.

See cockroachdb/cockroach#15228
@bdarnell
Copy link
Copy Markdown
Contributor Author

Added a new commit to update the acceptance tests (it won't pass until cockroachdb/postgres-test#22 is merged and tagged).

The propEvalKV rollout means we can no longer support upgrading from
versions older than 20170413. Update tests and remove some
no-longer-needed migrations.
This allows us to remove nearly all protobufs from the "below raft"
list of types requiring strict compatibility.
@bdarnell bdarnell force-pushed the below-raft-protos branch from ffe1a0c to 8871d35 Compare April 23, 2017 15:34
@bdarnell
Copy link
Copy Markdown
Contributor Author

Done, and I also swapped the order of the commits so the acceptance tests pass for both of them.

@tamird
Copy link
Copy Markdown
Contributor

tamird commented Apr 23, 2017

LGTM!

@bdarnell bdarnell merged commit a9aebe9 into cockroachdb:master Apr 23, 2017
@bdarnell bdarnell deleted the below-raft-protos branch April 23, 2017 15:57
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