Return disk errors back to client

[whyrusleeping]

I was looking at trying this out as it seems like it might fit a good usecase for me, but i'm not sure exactly how it should be used. Specifically, there doesnt seem to be any sort of error handling, what happens when things go wrong? A Set call can't always succeed, how will i know when it doesnt?

[barakmich]

Same also with NewKV and Close -- io.Closer is idiomatic, please follow it.

[manishrjain]

@whyrusleeping : The rationale that some of these operations return no error is that unless we are unable to write to disk, there's no reason for the Set to fail (in which case it would log.Fatal, because there's not much you could do if you can't write to disk). It's a blind write. The only operations which can fail are CAS ops, which check the previous state.

Though, I see the point that given this is a library, that choice of log.Fatal isn't one we should make. @ashwin95r is working to send disk errors back (and not log.Fatal).

[whyrusleeping]

Yeah, definitely don't want to be doing log.Fatals in lower layers of the app. Let us know when you've got that wired up through, i'm really excited to work with badger :)

[manishrjain]

@ashwin95r and I looked at the code again. Even if we change Set to return an error back, there's nothing much a user can do about it, because this relates to a file system failure. One can't just change the directory Badger is pointing to; because it would then stop working.

Similarly, Get can only fail if we're unable to read stuff off disk (which would cause lot more issues than just a request failure, because LSM trees do lot of things in the background). All of these are permanent failures; so I see little benefit in returning these to the user.

We could add io.Closer interface, if that's of any value. It would just return nil.

[whyrusleeping]

The point is that my app needs to know that something is wrong, and be able to shut down itself safely. If something deep within the datatstore calls a log.fatal, that's going to lead to dropped connections and corruptions of other things. Even if I can't do anything about badger failing to write, I still want to know when it falls

…

On Mon, May 15, 2017, 17:51 Manish R Jain ***@***.***> wrote: @ashwin95r <https://github.com/ashwin95r> and I looked at the code again. Even if we change Set to return an error back, there's nothing much a user can do about it, because this relates to a file system failure. One can't just change the directory Badger is pointing to; because it would then stop working. Similarly, Get can only fail if we're unable to read stuff off disk (which would cause lot more issues than just a request failure, because LSM trees do lot of things in the background). All of these are permanent errors; so I see little benefit in returning those. We could add io.Closer interface, if that's helpful. It would just return nil. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABL4HCll3ryLoDX9Vj7A7ZaSqm0sFENxks5r6PMogaJpZM4NbvX2> .

[whyrusleeping]

Also, another point is that I never want to return "success" to the user unless I know for certain their data is safe on disk

…

On Mon, May 15, 2017, 17:55 Jeromy Johnson ***@***.***> wrote: The point is that my app needs to know that something is wrong, and be able to shut down itself safely. If something deep within the datatstore calls a log.fatal, that's going to lead to dropped connections and corruptions of other things. Even if I can't do anything about badger failing to write, I still want to know when it falls On Mon, May 15, 2017, 17:51 Manish R Jain ***@***.***> wrote: > @ashwin95r <https://github.com/ashwin95r> and I looked at the code > again. Even if we change Set to return an error back, there's nothing > much a user can do about it, because this relates to a file system failure. > One can't just change the directory Badger is pointing to; because it would > then stop working. > > Similarly, Get can only fail if we're unable to read stuff off disk > (which would cause lot more issues than just a request failure, because LSM > trees do lot of things in the background). All of these are permanent > errors; so I see little benefit in returning those. > > We could add io.Closer interface, if that's helpful. It would just > return nil. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#28 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABL4HCll3ryLoDX9Vj7A7ZaSqm0sFENxks5r6PMogaJpZM4NbvX2> > . >

[awfm9]

It does not make sense to panic in the library. When saying it has to be a fatal error, you are ignoring that the consuming application might be doing a whole host of other things unrelated to writing to a K/V store. Bubbling up the errors to the user of the API and letting him decide what to do about it is definitely the way to go.

[manishrjain]

This is a duplicate of #23, which I just addressed. Have another look at the public APIs, and feel free to reopen if I missed some API, or you expect to see something more.