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.

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

consumer: return descriptive error when a nil state is passed into NewJSONFileStore #245

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

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.

None yet

2 participants