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/mvcc: extract mvcc subpackage to break dependency on pebble #64468

Closed

Conversation

ajwerner
Copy link
Contributor

@ajwerner ajwerner commented Apr 30, 2021

We have some rules about which packages are allowed to import which package. In particular, there's some assumptions that some bulk io code not directly import storage. That code relies on some definitions of things like MVCCSimplerIterator and MVCCKeyValue. These two commits pull some symbols out of storage and into storage/mvcc and then renames prefixes of symbols to remove the leading MVCC. All of this code was generated using the automated refactorings in GoLand. No logic was changed.

Part of #64450.

Release note: None

The dependency on these concepts was creating unintended dependencies on
things like storage and pebble.

Release note: None
The linter was not happy about these.

Release note: None
@ajwerner ajwerner requested review from itsbilal, sumeerbhola and a team April 30, 2021 13:21
@ajwerner ajwerner requested a review from a team as a code owner April 30, 2021 13:21
@ajwerner ajwerner requested review from a team and dt and removed request for a team April 30, 2021 13:21
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner
Copy link
Contributor Author

This one is likely controversial. @dt says he doesn't care about the storage dep from bulk io so I'm going to nix this.

@ajwerner ajwerner closed this Apr 30, 2021
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.

Reviewed 8 of 134 files at r1, 12 of 101 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/storage/engine.go, line 58 at r2 (raw file):

	// iteration. After this call, valid will be true if the iterator was not
	// originally positioned at the last key. Note that unlike
	// Iterator.NextKey, this method does not skip other versions with the

mvcc.Iterator.NextKey


pkg/storage/engine.go, line 60 at r2 (raw file):

	// Iterator.NextKey, this method does not skip other versions with the
	// same EngineKey.Key.
	// TODO(sumeer): change Iterator.Next() to match the

ditto


pkg/storage/engine.go, line 130 at r2 (raw file):

	//
	// These fields are only relevant for MVCCIterators. Additionally, an
	// Iterator with timestamp hints will not see separated intents, and may

ditto.
also can you search for plurals like MVCCIterators, since they don't get fixed automatically.


pkg/storage/engine_test.go, line 161 at r2 (raw file):

				}

				// Iterator should not reuse its cached result.

mvcc.Iterator

Similarly in other files, like intent_interleaving_iter.go


pkg/storage/mvcc/doc.go, line 11 at r1 (raw file):

// licenses/APL.txt.

// Package mvcc exports interfaces for interacting with mvcc storage.

Could you add a bit more context here, like what is said in the PR description about dependencies. Otherwise someone may imagine that all interfaces for interacting with mvcc storage belong here, while in practice what is here is a (smallish) subset, and it may create confusion on where to place future additions.

@sumeerbhola
Copy link
Collaborator

never mind, now that this is closed

@ajwerner
Copy link
Contributor Author

Thanks for giving it a look. Sorry for the noise!

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.

3 participants