consumer: return descriptive error when a nil state is passed into NewJSONFileStore#245
consumer: return descriptive error when a nil state is passed into NewJSONFileStore#245kanwals wants to merge 4 commits intogazette:masterfrom
Conversation
jgraettinger
left a comment
There was a problem hiding this comment.
Thanks. This is actually a case where i'd mostly focus on testing, because a defect here can silently persist bad data that's only discovered much later, as you saw (aka, potential for a big production headache). Please also update godocs since we're putting a new constraint on callers.
Reviewable status: 0 of 1 files reviewed, all discussions resolved
|
Just a headsup that I'm aware of the response, and would likely need some time (probably EOW) to get to this. |
kanwals
left a comment
There was a problem hiding this comment.
Took a stab at it. I wasn't completely sure of what the right location of the test should be, and LMK if this doesn't look good.
PTAL @jgraettinger
Reviewable status: 0 of 4 files reviewed, all discussions resolved
consumer/shard.go, line 248 at r3 (raw file):
s.wg.Wait() if s.store != nil && !reflect.ValueOf(s.store).IsNil() {
This one was a gotcha, where control would still go inside the if since s.store was a typed nil.
Return a more descriptive error if a
nilstateis passed intoNewJSONFileStore.Didn't add/modify tests as the change is pretty trivial. LMK if you think otherwise.
This change is