batcheval: absorb command registration

[tbg]

You'll notice that EndTransaction is still lingering in replica_command.go,
but that will be done in a follow-up.

Touches #18779.

[cockroach-teamcity]

This change is 

[nvanbenschoten]

This cleanup is really satisfying to follow!

---

Reviewed 38 of 38 files at r1.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

---

pkg/storage/cclglue.go, line 25 at r1 (raw file):

)

// TODO(tschottdorf): seems straightforward enough to shoehorn this into

Why not handle this here as well?

---

pkg/storage/helpers_test.go, line 474 at r1 (raw file):

	}

	batcheval.RegisterCommand(roachpb.AddSSTable, nil, nil)

Why are we doing this twice?

Never mind, but I'd still push for an UnregisterCommand function.

---

pkg/storage/replica_command.go, line 160 at r1 (raw file):

func init() {
	batcheval.RegisterCommand(roachpb.EndTransaction, declareKeysEndTransaction, evalEndTransaction)

I know you're going to get to this soon, but could you add a TODO here?

---

pkg/storage/batcheval/cmd_begin_transaction.go, line 34 at r1 (raw file):

}

// DeclareKeysWriteTransaction is the shared portion of

Consider a TODO to unexport this when EndTxn is moved into batcheval.

---

pkg/storage/batcheval/cmd_conditional_put.go, line 29 at r1 (raw file):

}

// ConditionalPut sets the value for a specified key only if

Do these functions need to be exported?

---

pkg/storage/batcheval/cmd_deprecated_verify_checksum.go, line 26 at r1 (raw file):

func init() {
	RegisterCommand(roachpb.DeprecatedVerifyChecksum, DefaultDeclareKeys, deprecatedVerifyChecksum)

nit: Why declare any keys for this command?

---

pkg/storage/batcheval/cmd_lease.go, line 32 at r1 (raw file):

func declareKeysRequestLease(
	desc roachpb.RangeDescriptor, header roachpb.Header, req roachpb.Request, spans *spanset.SpanSet,

The linter might yell at you about this long line.

---

pkg/storage/batcheval/cmd_resolve_intent_test.go, line 36 at r1 (raw file):

	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
	"github.com/cockroachdb/cockroach/pkg/util/uuid"
	opentracing "github.com/opentracing/opentracing-go"

nit: move up

---

pkg/storage/batcheval/cmd_scan.go, line 30 at r1 (raw file):

type Command struct {
	// DeclareKeys adds all keys this command touches to the given spanSet.
	DeclareKeys func(roachpb.RangeDescriptor, roachpb.Header, roachpb.Request, *spanset.SpanSet)

Consider creating typedefs of these two functions and sharing between here and RegisterCommands to avoid duplication.

---

pkg/storage/batcheval/cmd_scan.go, line 52 at r1 (raw file):

	impl func(context.Context, engine.ReadWriter, CommandArgs, roachpb.Response) (result.Result, error),
) {
	if declare == nil && impl == nil {

Why not just create a UnregisterCommand function? Seems less obscure.

---

Comments from Reviewable

[tbg]

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

pkg/storage/cclglue.go, line 25 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Why not handle this here as well?

I take it back, importCmd is an admin command. Removed the TODO.

---

pkg/storage/helpers_test.go, line 474 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Why are we doing this twice?

Never mind, but I'd still push for an UnregisterCommand function.

Done.

---

pkg/storage/replica_command.go, line 160 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I know you're going to get to this soon, but could you add a TODO here?

Done.

---

pkg/storage/batcheval/cmd_begin_transaction.go, line 34 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Consider a TODO to unexport this when EndTxn is moved into batcheval.

Done. (elsewhere)

---

pkg/storage/batcheval/cmd_conditional_put.go, line 29 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do these functions need to be exported?

no, but I decided to leave them exported because they are the meat of what this package provides. We don't really spend a lot of time looking at godoc, but if we did, we'd want to see them and their comments.

---

pkg/storage/batcheval/cmd_deprecated_verify_checksum.go, line 26 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: Why declare any keys for this command?

You could take it a step further and ask "why not delete this command". We can probably delete this, but I'd rather not do it as a drive-by and have thus opted to preserve the existing code verbatim.

---

pkg/storage/batcheval/cmd_lease.go, line 32 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

The linter might yell at you about this long line.

The linter and I are friends.

---

pkg/storage/batcheval/cmd_resolve_intent_test.go, line 36 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: move up

Done.

---

pkg/storage/batcheval/cmd_scan.go, line 30 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Consider creating typedefs of these two functions and sharing between here and RegisterCommands to avoid duplication.

I'm not a huge fan of typedefs for fns because all they usually do is add an extra hop to look at what you're actually supposed to pass. I'll leave as is in this instance.

---

pkg/storage/batcheval/cmd_scan.go, line 52 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Why not just create a UnregisterCommand function? Seems less obscure.

Done.

---

Comments from Reviewable

[nvanbenschoten]

---

Reviewed 10 of 10 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from Reviewable