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.

Sign up for GitHub

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

Jump to bottom

core: refactoring projects #18779

Closed
1 of 12 tasks
cuongdo opened this issue Sep 26, 2017 · 6 comments
Closed
1 of 12 tasks

core: refactoring projects #18779

cuongdo opened this issue Sep 26, 2017 · 6 comments
Labels
C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. meta-issue Contains a list of several other issues.

Comments

@cuongdo
Copy link
Contributor

cuongdo commented Sep 26, 2017 •
edited by nvanbenschoten
Loading

This is a master list for core refactoring work. The theme is breaking up the massive storage package. Separate GitHub issues should be created as work or discussion begins on an item.

Ideas:

  • Break apart the larger types, like Replica.
    • Remove unrelated stuff from Replica.
  • Simplify Pre-Raft / Raft flow.
  • Add explicit instructions, such as bumping the transaction timestamp, that KV layer can send to TxnCoordSender. This would add grepability and understandablity.
  • Pull queues and caches into their own package.
  • Pull node liveness out of storage.
  • Centralize split code.
  • Add file-level comments that describe the general purpose and scope of that file.
  • Move RaftTransport out of storage
  • Move raftScheduler out of storage
  • storage: move evaluation of BatchRequests to a separate package #18784: Move evaluation of BatchRequests to a separate package
  • Enforce lock ordering, preferably programmatically

As you start to work on items, please put your name or GitHub id next to the task to claim it.
@tbg tbg mentioned this issue Sep 26, 2017
Closed
@tbg tbg mentioned this issue Sep 26, 2017
Closed
tbg added a commit to tbg/cockroach that referenced this issue Sep 26, 2017
@tbg
[DNM] factor out evaluation of BatchRequest
8711c74 
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.
@a6802739
Copy link
Contributor

@cuongdo, could I have a try for this part?

@petermattis
Copy link
Collaborator

@a6802739 The above checklist is a grab bag of refactoring ideas, not all of which may pan out. Give us some time to make a bit of progress on the margins and get a better feel for what the refactoring will look like. I'm sure there will be some tasks that emerge for you to tackle.

@nvanbenschoten nvanbenschoten mentioned this issue Sep 27, 2017
Merged
@a6802739
Copy link
Contributor

@petermattis, Thank you very much.

@a-robinson
Copy link
Contributor

Also, as suggested in #18657 (comment): remove the locking of Replica.raftMu in Replica.propose

@tbg
Copy link
Member

tbg commented Oct 9, 2017

#19133 has some more abstract refactoring ideas on how to make the "non-raft" portion of Replica clearer.

@a-robinson a-robinson mentioned this issue Oct 10, 2017
Closed
@nvanbenschoten nvanbenschoten mentioned this issue Oct 17, 2017
Merged
@a6802739 a6802739 self-assigned this Oct 24, 2017
@tbg tbg mentioned this issue Oct 29, 2017
Merged
tbg added a commit to tbg/cockroach that referenced this issue Oct 29, 2017
@tbg
storage: interface *Replica to ReplicaEvalContext
eb0045e 
Introduce an interface implemented by `*Replica` and used by
`ReplicaEvalContext` to prepare the way for moving `ReplicaEvalContext` into a
subpackage of `storage`.

See cockroachdb#18779.
tbg added a commit to tbg/cockroach that referenced this issue Oct 30, 2017
@tbg
storage: interface *Replica to ReplicaEvalContext
4086826 
Introduce an interface implemented by `*Replica` and used by
`ReplicaEvalContext` to prepare the way for moving `ReplicaEvalContext` into a
subpackage of `storage`.

See cockroachdb#18779.
@nvanbenschoten nvanbenschoten mentioned this issue Nov 13, 2017
Merged
tbg added a commit to tbg/cockroach that referenced this issue Nov 29, 2017
@tbg
batcheval: absorb command registration
2651d00 
You'll notice that `EndTransaction` is still lingering in `replica_command.go`,
but that will be done in a follow-up.

Touches cockroachdb#18779.
@tbg tbg mentioned this issue Nov 29, 2017
Merged
tbg added a commit to tbg/cockroach that referenced this issue Nov 30, 2017
@tbg
batcheval: absorb command registration
7a52a3a 
You'll notice that `EndTransaction` is still lingering in `replica_command.go`,
but that will be done in a follow-up.

Touches cockroachdb#18779.
tbg added a commit to tbg/cockroach that referenced this issue Nov 30, 2017
@tbg
batcheval: absorb command registration
98419a5 
You'll notice that `EndTransaction` is still lingering in `replica_command.go`,
but that will be done in a follow-up.

Touches cockroachdb#18779.
@nvanbenschoten nvanbenschoten mentioned this issue Dec 12, 2017
Merged
@bdarnell bdarnell mentioned this issue Dec 14, 2017
Merged
@petermattis petermattis added meta-issue Contains a list of several other issues. C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. labels Jul 21, 2018
@tbg
Copy link
Member

tbg commented Jul 22, 2018

Closing as there isn't anything very specific left here.

@tbg tbg closed this as completed Jul 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. meta-issue Contains a list of several other issues.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@cuongdo @a6802739 @tbg @petermattis @a-robinson