Skip to content

Conversation

@kanwals
Copy link

@kanwals kanwals commented Dec 4, 2019

Return a more descriptive error if a nil state is passed into NewJSONFileStore.

Didn't add/modify tests as the change is pretty trivial. LMK if you think otherwise.


This change is Reviewable

Copy link
Contributor

@jgraettinger jgraettinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kanwals
Copy link
Author

kanwals commented Dec 10, 2019

Just a headsup that I'm aware of the response, and would likely need some time (probably EOW) to get to this.

Gurkanwal Singh Batra added 3 commits December 18, 2019 01:52
- refactor godoc comment
Copy link
Author

@kanwals kanwals left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants