Use failure for error management

[sticnarf]

As mentioned in #10, we are going to use failure for error management.

This PR is almost a mechanical transformation of @sunxiaoguang 's previous work, leaving furthur questions to other PRs.

Questions about current errors:

• Should ErrorKind be public to users? Currently, I think the caller can do nothing effective at runtime when receiving an Error (except NoSuchKey, discussed below). The caller can just log the error and try again. Therefore, it seems useless to expose ErrorKind to users. Keeping it private helps with forward compatibility, however we may not need that.

• Should we treat NoSuchKey as an error? People might need to know whether a key exists. Now, they have to match from the error variants and see if it is a NoSuchKey. IMO Option<Value> is a better option for the get api.

• InternalError looks like a box for all uncategorized errors. Those which relate to PD should have their own error variants in the future.

• Inconsistency with errors from kvproto. KeyError is just a wrapper of the original error, and the others extracts the inner fields.

[sunxiaoguang]

Thanks, I will try to come across this at a later time.

[sunxiaoguang]

As mentioned in #10, we are going to use failure for error management.

This PR is almost a mechanical transformation of @sunxiaoguang 's previous work, leaving furthur questions to other PRs.

Questions about current errors:

• Should ErrorKind be public to users? Currently, I think the caller can do nothing effective at runtime when receiving an Error (except NoSuchKey, discussed below). The caller can just log the error and try again. Therefore, it seems useless to expose ErrorKind to users. Keeping it private helps with forward compatibility, however we may not need that.
• Should we treat NoSuchKey as an error? People might need to know whether a key exists. Now, they have to match from the error variants and see if it is a NoSuchKey. IMO Option<Value> is a better option for the get api.
• InternalError looks like a box for all uncategorized errors. Those which relate to PD should have their own error variants in the future.
• Inconsistency with errors from kvproto. KeyError is just a wrapper of the original error, and the others extracts the inner fields.

I totally agree that Get should return an Option instead of error for this case. Let's change it to what you just suggested.

Although I'm still not quite confident about the whole error things yet, but I agree that users usually do not care the detail error types. The whole reason behind structured error types was for the client itself to properly handle certain errors, for example automatically retried or refresh region cache etc. As we are not working on transactional client yet, we have plenty of time to decide what we are going to do for transactional api.

[Hoverbear]

Regarding these:

• Should ErrorKind be public to users? Currently, I think the caller can do nothing effective at runtime when receiving an Error (except NoSuchKey, discussed below). The caller can just log the error and try again. Therefore, it seems useless to expose ErrorKind to users. Keeping it private helps with forward compatibility, however we may not need that.

Although I'm still not quite confident about the whole error things yet, but I agree that users usually do not care the detail error types. The whole reason behind structured error types was for the client itself to properly handle certain errors, for example automatically retried or refresh region cache etc. As we are not working on transactional client yet, we have plenty of time to decide what we are going to do for transactional api.

Seems ErrorKind is commonly public:

• https://doc.rust-lang.org/std/io/enum.ErrorKind.html
• https://doc.rust-lang.org/core/num/enum.IntErrorKind.html
• https://docs.rs/rand/0.5.0/rand/enum.ErrorKind.html

I'd also note that the failure docs do say this is forward compatible: https://github.com/rust-lang-nursery/failure/blob/master/book/src/error-errorkind.md

I think catering to the usual case is usually fine, but we must also be able to cater to the unusual case for some users. If this is as simple as making ErrorKind public, I don't think it's a huge problem.

[sticnarf]

@Hoverbear

I'd also note that the failure docs do say this is forward compatible: https://github.com/rust-lang-nursery/failure/blob/master/book/src/error-errorkind.md

I think the forward compatibility that failure doc claims is achieved by keeping ErrorKind private. Suppose ErrorKind is public, and the crate user does an exhaustive match, then we adding a variant breaks the program.

I think catering to the usual case is usually fine, but we must also be able to cater to the unusual case for some users. If this is as simple as making ErrorKind public, I don't think it's a huge problem.

I agree we probably need to support those unusual users. I am neutral about the visibility of ErrorKind. I am going to make it public if there aren't any other worries.

[Hoverbear]

and the crate user does an exhaustive match, then we adding a variant breaks the program.

In this case they are explicitly opting into this breaking because they want to exhaustively match. If they want to avoid this they can add a _ => {} match.

[sticnarf]

and the crate user does an exhaustive match, then we adding a variant breaks the program.

In this case they are explicitly opting into this breaking because they want to exhaustively match. If they want to avoid this they can add a _ => {} match.

Yes. That's rare and can be avoided. So the compatibility is not a big problem.

[sticnarf]

Merged master and NoSuchKey has been removed.
@sunxiaoguang PTAL

[Hoverbear]

LGTM

[brson]

For backward compatibility with the ErrorKind enum you could make it an "open enum", by just adding a __DoNotMatch variant to it (or whatever the prevailing wisdom is for marking open enums).

But it looks good to me. While I do like most of the failure crate, the amount of boilerplate is a drag.

Also, I still don't see the advantage of Error + ErrorKind over error enums, even after reading https://rust-lang-nursery.github.io/failure/error-errorkind.html.

LGTM.

[Hoverbear]

@sticnarf We have a conflict. 😭 Do you think you can resolve it then we can merge?

[sticnarf]

@Hoverbear Done

[Hoverbear]

Force merging due to CI.

[Hoverbear]

THanks for this contribution @sticnarf , I really appreciate it. :) It's so nice to have folks like you involved.