notice @bdarnell

Reviewed 13 of 13 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

sql/main_test.go, line 102 [r1] (raw file):
I think this is over-engineered - simply make replay protection a flag on the CommandFilters struct whose effect is never giving a replay to the filter implementation. Populating a map will be cheap enough for any type of reasonable unit test.

storage/replica.go, line 78 [r1] (raw file):
the right way to do this is without an allocation every time is

type raftCmdIDContextKeyType struct{}
var raftCmdIDContextKey raftCmdIDContextKeyType

storage/replica.go, line 1325 [r1] (raw file):
what is it with those ?!?? This isn't a 14 year old's diary. 😄

storage/replica.go, line 1353 [r1] (raw file):
do this only if a TestingCommandFilter is set.

storage/replica.go, line 1632 [r1] (raw file):
This probably forces the index on the heap, so we allocate one item for each individual request in the batch. Make this conditional on having a TestingCommandFilter in place, and move the other WithValue next to it.

storage/replica.go, line 2036 [r1] (raw file):
r.context()

Comments from the review on Reviewable.io

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

sql/main_test.go, line 102 [r1] (raw file):
I think I'd rather keep it like this; it's a better api forcing you to think about idempotency at the time you install a filter. Because otherwise we probably wouldn't set that flag by default, and then you'd forget to set it if you need it, etc. Also I don't think it'd save much code.

storage/replica.go, line 78 [r1] (raw file):
Done.

storage/replica.go, line 1325 [r1] (raw file):
SNAP! I was differentiating between an unexpected condition and an unrecoverable condition. But fine.

storage/replica.go, line 1353 [r1] (raw file):
Done.

storage/replica.go, line 1632 [r1] (raw file):
Well I can't move the other WithValue, because I don't have a cmdID here, and the read call paths generally don't have such an id, right?
I've made this line conditional on TestingCommandFilter.
To avoid a new allocation here, I could put the whole struct{key, index} in the context in processRaftCommand, with a dummy index val, and the modify that struct here. But then I'd have to put a *struct in the context... I don't think it's worth it.

storage/replica.go, line 2036 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

mod making replay protection a bool on the CommandFilter struct.

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

sql/main_test.go, line 102 [r1] (raw file):
No, I was thinking to leave replay protection off by default. There's no indication of any other test except yours in the system that has replay protection flakiness (though it does happen occasionally), and the code in *CommandFilters is already a mouthful to read. KISS

storage/replica.go, line 78 [r1] (raw file):
I don't see it (or any other updates).

storage/replica.go, line 1632 [r1] (raw file):
ah, yeah, no such thing on reads. With it being conditional on a mock set, allocs don't matter, so that's fine.

Comments from the review on Reviewable.io

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

storage/replica.go, line 1310 [r2] (raw file):
Give this a different name (RaftCmdIDAndIndex?), because it's more than just the command ID.

storage/replica_command.go, line 62 [r2] (raw file):
I'd rather make commandID and index arguments to the filter than pass them around through the context.Context. If you're concerned about the number of arguments and needing to touch every filter every time you change the signature you could put all the arguments into a FilterContext struct, so you can see what's there instead of the context.Context black box.

Passing the context.Context around may be a good thing for other reasons (deadlines, cancellation), but passing necessary data through it is a bad idea.

Comments from the review on Reviewable.io

Review status: 0 of 16 files reviewed at latest revision, 3 unresolved discussions.

sql/main_test.go, line 102 [r1] (raw file):
i've extracted the replay protection into a CommandFilter wrapper that anyone can use. PTAL.

storage/replica.go, line 1310 [r2] (raw file):
Done.

storage/replica_command.go, line 62 [r2] (raw file):
ok, passing explicitly now.
Left the context too, seems like a good idea to pass it around.

Comments from the review on Reviewable.io

Review status: 0 of 16 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

storage/util/util.go, line 17 [r4] (raw file):
Why did this need to be extracted into a separate package? Do we ever use a command filter from a package that's not allowed to import storage?

The package should be named storage/storageutil (see io/ioutil in the standard library) so that you don't have to manually add an import alias everywhere you use it.

storage/util/util.go, line 23 [r4] (raw file):
Why introduce a struct (with a name that precludes any future flexibility) instead of passing two separate arguments?

testutils/mocking.go, line 25 [r4] (raw file):
The testutils package should have minimal dependencies; this should go into a subpackage like testuils/gossiputil and testutils/sqlutils (ugh, the inconsistent pluralization). Or I think it would be fine to put this in storage itself since that's where the command filter mechanism is defined.

Comments from the review on Reviewable.io

Reviewed 16 of 16 files at r4.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

storage/replica.go, line 1081 [r4] (raw file):
storage_util strikes me as an unidiomatic package name.

storage/replica_command.go, line 62 [r2] (raw file):
Makes little sense to pass the context down into executeCmd if you ask me. Anything above is fine. We should generally not pass things around unless there's actually a reason to use it.

storage/util/util.go, line 23 [r4] (raw file):
it's also only used during mocks - that's why I agreed to putting it in the context initially. It's awkward to plumb something down three levels just to use it in a mock. I'll defer to your judgment here though, somehow all options are a little unsavory.

testutils/mocking.go, line 28 [r4] (raw file):
Seems superfluous.

Comments from the review on Reviewable.io

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

testutils/mocking.go, line 28 [r4] (raw file):
I had to do it go get around a lint that wanted error to be the last thing returned from functions

Comments from the review on Reviewable.io

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

testutils/mocking.go, line 28 [r4] (raw file):
Where? I don't see that function. If it's required, add a comment explaining that.

Comments from the review on Reviewable.io

Review status: 0 of 16 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

storage/replica.go, line 1081 [r4] (raw file):
changed to storagebase

storage/replica_command.go, line 62 [r2] (raw file):
I've hid it now in a FilterArgs struct. Doesn't hurt anything, and a filter sounds like the kind of thing that would like a Context...

testutils/storageutils/mocking.go, line 25 [r4] (raw file):
ok, created testutils/storageutils. I wonder if these all should have been testutils/storagetestutils

testutils/storageutils/mocking.go, line 28 [r4] (raw file):
Added a comment.

storage/util/util.go, line 17 [r4] (raw file):
yeah, it's used in package sql, and sql can't import storage because, believe it or not, storage imports sql and breaking that was harder.

storage/util/util.go, line 23 [r4] (raw file):
ok, made the struct private.

Comments from the review on Reviewable.io

LGTM. I'm not sure that the storagebase package is worth it (the one type there could be replaced with a plain string) but there's been enough bikeshedding here and this looks fine.

Review status: 0 of 16 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

Comments from the review on Reviewable.io