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

Errors in the API #23

Closed
fjl opened this issue May 15, 2017 · 11 comments
Closed

Errors in the API #23

fjl opened this issue May 15, 2017 · 11 comments

Comments

@fjl
Copy link
Contributor

fjl commented May 15, 2017

I'm trying badger as a replacement for github.com/syndtr/goleveldb and one thing that hit me immediately is that there are no errors in the badger Go API. Any error will just exit the process through log.Fatalf. Is that a design decision or just temporary?

@fjl
Copy link
Contributor Author

fjl commented May 15, 2017

I see that there is a way to get errors when using Entry. Would still be nice to have a version of NewKV that doesn't exit the program.

@manishrjain
Copy link
Contributor

log.Fatals only happen when Badger itself causes issues. External input won't cause it. In fact, these Fatals are really great at catching bugs and making the library robust.

We send errors back when justified, for e.g. when doing CAS operations.

@fjl
Copy link
Contributor Author

fjl commented May 15, 2017

badger.NewKV("non-existent-dir/foo")

crashes my program. That's not acceptable. Opening a non-existent file is not an internal error, it's a user error which my program will want to relay to the user in an arbitrary way.

@manishrjain manishrjain reopened this May 16, 2017
@szymonm szymonm self-assigned this May 16, 2017
@szymonm
Copy link
Contributor

szymonm commented May 16, 2017

We are adding basic checks for the directory to ease usage for the newcomers, see #29.
Overall, we decided that it's library user's responsibility to provide a writable directory for badger for the whole duration of using KV.

There is also a separate thread for disk errors: #28

@whyrusleeping
Copy link

Any operation that could fail, under any conditions, should return an error.

we decided that it's library user's responsibility to provide a writable directory

Sure, you can hope the user provides a writeable directory, but the fact remains that if they give you a directory, and you can't write to it, you need to tell them they failed on their end of the contract.

@szymonm
Copy link
Contributor

szymonm commented May 17, 2017

I see your point, you are right. We can do much better error handling.

@szymonm szymonm removed their assignment May 17, 2017
@jbowens
Copy link
Contributor

jbowens commented May 17, 2017

Besides the pragmatic concerns, it violates go conventions to panic across a package boundary.

@manishrjain
Copy link
Contributor

Yup. Agree with all the concerns here. We have started to surface errors via the API calls (see 31c0bd2). We'll do a sweep over the next couple of weeks over the entire code base. And try to remove all calls to y.Checkf, if possible.

The biggest hurdle would be to deal with background tasks, and we'll need to build mechanisms to deal graciously with them. I'll keep you updated on this thread.

@awfm9
Copy link

awfm9 commented May 22, 2017

Good to hear, looking forward to using the library.

@manishrjain
Copy link
Contributor

See changes: 55c350d and 7610c2f. These surface error handling all the way from disk to public APIs. They should address the concerns raised in this issue.

Closing the issue. Feel free to reopen if some API is still unhandled.

@awfm9
Copy link

awfm9 commented May 31, 2017

Excellent. I will probably switch our usage of BoltDB to Badger, since we are in the early stages of our project as well.

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

No branches or pull requests

6 participants