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

KV parameters verification. #29

Closed
wants to merge 2 commits into from
Closed

KV parameters verification. #29

wants to merge 2 commits into from

Conversation

szymonm
Copy link
Contributor

@szymonm szymonm commented May 16, 2017

Resolves #23


This change is Reviewable

@szymonm szymonm requested a review from manishrjain May 16, 2017 08:46
@szymonm szymonm mentioned this pull request May 16, 2017
badger/kv.go Outdated
return nil, y.Wrapf(err, "Incorrect Dir parameter")
}
if !dirExists {
return nil, y.Errorf("Directory '%s' does not exist.", opt.Dir)
Copy link

Choose a reason for hiding this comment

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

Use %q instead of '%s'

badger/kv.go Outdated
return nil, y.Errorf("Directory '%s' does not exist.", opt.Dir)
}
if !(opt.ValueLogFileSize <= 2<<30 && opt.ValueLogFileSize >= 1<<20) {
return nil, y.Errorf("ValueLogFileSize should be between 1MB and 1GB")
Copy link

Choose a reason for hiding this comment

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

Replace should with must as you are handling this as a fatal issue.

@manishrjain
Copy link
Contributor

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


badger/kv.go, line 125 at r1 (raw file):

Previously, dolmen (Olivier Mengué) wrote…

Use %q instead of '%s'

Address the comment above. Also, predefine this error using var, and just reuse them. That way, a user can check the error returned against the var, instead of having to do string matching.

both of these above errors can be ErrInvalidDir.


badger/kv.go, line 198 at r1 (raw file):

		return true
	}
	out.vlog.Replay(vptr, fn)

Does Replay return any error? If it can, we should handle it here.

In fact, if you look carefully at all the y.Checkf, you should be able to find the ones which are being directly called from the API. Make all those return errors using errors.Wrapf and the likes. I did that just now for value log opening and closing.


Comments from Reviewable

@szymonm
Copy link
Contributor Author

szymonm commented May 19, 2017

Review status: 2 of 5 files reviewed at latest revision, 3 unresolved discussions.


badger/kv.go, line 125 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Address the comment above. Also, predefine this error using var, and just reuse them. That way, a user can check the error returned against the var, instead of having to do string matching.

both of these above errors can be ErrInvalidDir.

Added type instead of var in order to pass the error message up.
It still can be checked without string matching.


badger/kv.go, line 128 at r1 (raw file):

Previously, dolmen (Olivier Mengué) wrote…

Replace should with must as you are handling this as a fatal issue.

Done.


badger/kv.go, line 198 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Does Replay return any error? If it can, we should handle it here.

In fact, if you look carefully at all the y.Checkf, you should be able to find the ones which are being directly called from the API. Make all those return errors using errors.Wrapf and the likes. I did that just now for value log opening and closing.

Leaving this for you.


Comments from Reviewable

@manishrjain
Copy link
Contributor

:lgtm: one small comment.


Reviewed 1 of 3 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


badger/kv.go, line 120 at r3 (raw file):

var ErrInvalidDir error = errors.New("Invalid Dir, directory does not exist")
var ErrInvalidValueLogFileSize error = errors.New("Invalid ValueLogFileSize, must be between 1MB and 1GB")

ErrValueLogSize


Comments from Reviewable

@szymonm
Copy link
Contributor Author

szymonm commented May 19, 2017

Merged.

@szymonm szymonm closed this May 19, 2017
@deepakjois deepakjois deleted the paramsCheck branch October 30, 2017 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants