[DNM] storage: factor out evaluation of BatchRequest #18796
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is one of the self-contained threads to unravel for cockroachdb#18784.
This is also required for cockroachdb#18784.
|
This change is |
See cockroachdb#18779. This doesn't carry out all the work but does all the basic required refactoring (mod cleanups). I don't love that we're passing `ReplicaEvalContext` through an interface. That should be avoidable and doing so should be instructive (I suspect we'll trim some fat in the process): Many methods roughly take the following form: ```go // GetMVCCStats returns the Replica's MVCCStats. func (rec ReplicaEvalContext) GetMVCCStats() enginepb.MVCCStats { r := rec.repl r.mu.RLock() defer r.mu.RUnlock() return *r.mu.state.Stats, nil } ``` and to define this method inside of the `batcheval` package, we'd have instead ```go type ReplicaEvalContext { // ... state struct { *sync.Mutex *storagebase.ReplicaState } } ``` though having to copy the mutex pointer smells fishy. Similar strategies can be used for most of the other methods. The downside is that `ReplicaEvalContext` will wind up being a struct with lots of pointers (i.e., perhaps a little large to sit in the hot path, though much smaller than BatchRequest!). We could accommodate that by arranging some of these pointers that never change (for example the `Engine` or the `AbortCache`) into a long-lived struct on `*Replica` instead.
|
Very nice! I'm not in love with the name Review status: 0 of 30 files reviewed at latest revision, all discussions resolved, some commit checks failed. Comments from Reviewable |
|
Let the bike shed begin, @petermattis. I'd prefer not having to carry out a rename once I brush this up. I like |
|
|
|
I like |
|
I'm also ok with |
|
Superseded by #19888. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The first two commits are #18785.
See #18779.
This doesn't carry out all the work but does all the basic required
refactoring (mod cleanups).
I don't love that we're passing
ReplicaEvalContextthrough an interface.That should be avoidable and doing so should be instructive (I suspect
we'll trim some fat in the process):
Many methods roughly take the following form:
and to define this method inside of the
batchevalpackage, we'd haveinstead