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

storage: remove unnecessary sideload inlining, add assertion #20573

Merged

Conversation

nvanbenschoten
Copy link
Member

Related to #17500.

Replica.sendRaftMessages used to contain logic to inline sideloaded entries
for MsgApps. This was unnecessary, as MsgApps would never contain thin entries.
This is because replicaRaftStorage.Entries already performs the sideload inlining
for stable entries and raft.unstable always contain fat entries. Since these are
the only two sources that raft.sendAppend gathers entries from to populate MsgApps,
we should never see thin entries in MsgApps.

This change replaces the inlining attempt with an assertion.

Release note: None

@nvanbenschoten nvanbenschoten requested review from tbg and a team December 7, 2017 23:33
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@bdarnell
Copy link
Contributor

:lgtm:


Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


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

		case raftpb.MsgApp:
			// FOR REVIEW: this assertion will be quick for all non-sideloaded
			// commands. Is it worth always performing the assertion?

I could go either way but I guess I'd lean towards keeping this assertion RaceEnabled-only.


Comments from Reviewable

@tbg
Copy link
Member

tbg commented Dec 11, 2017

:lgtm: and thanks!


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


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

Previously, bdarnell (Ben Darnell) wrote…

I could go either way but I guess I'd lean towards keeping this assertion RaceEnabled-only.

What Ben said. The recipient would find out pretty quickly that they're screwed, too.


Comments from Reviewable

`Replica.sendRaftMessages` used to contain logic to inline sideloaded entries
for `MsgApps`. This was unnecessary, as `MsgApps` would never contain thin entries.
This is because `replicaRaftStorage.Entries` already performs the sideload inlining
for stable entries and `raft.unstable` always contain fat entries. Since these are
the only two sources that `raft.sendAppend` gathers entries from to populate `MsgApps`,
we should never see thin entries in `MsgApps`.

This change replaces the inlining attempt with an assertion.

Release note: None
@nvanbenschoten
Copy link
Member Author

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


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

Previously, tschottdorf (Tobias Schottdorf) wrote…

What Ben said. The recipient would find out pretty quickly that they're screwed, too.

Done.


Comments from Reviewable

@nvanbenschoten nvanbenschoten merged commit 2185fa9 into cockroachdb:master Dec 11, 2017
@nvanbenschoten nvanbenschoten deleted the nvanbenschoten/fatEntry branch December 11, 2017 23:22
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