New subcode for IOError to detect the ESTALE errno

[mdlugajczyk]

I'd like to propose a patch to expose a new IOError type with subcode kStaleFile to allow to detect when ESTALE error is returned. This allows the rocksdb consumers to handle this error separately from other IOErrors.

I've also added a missing string representation for the kDeadlock subcode, I believe calling ToString() on Status object with that subcode would result in an out of band access in the msgs array,

Please let me know if you have any questions or would like me to make any changes to this pull request.

[facebook-github-bot]

@IslamAbdelRahman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[IslamAbdelRahman]

Thanks @mdlugajczyk, Can you please explain why you need this

[mdlugajczyk]

@IslamAbdelRahman - I'm storing rocksdbs on a distributed file system, if it becomes unavailable rocksdb receives ESTALE errno and returns IOError. I want to handle this situation, but don't want this code to be triggered by any other type of IOError.

I could try checking the error message of IOError and check for existence of "stale file" substring, but I don't like the idea of parsing error messages to determine what error has occurred, particularly since rocksdb had this information.

Does this explain the use case?

[IslamAbdelRahman]

Thanks @mdlugajczyk for explaining.

Can you simply check status.subcode() in your application.
I am just asking because there are many possible errors that could happen and it does not make sense to put a special Status class for each one of them, so I think it would be better to check the subcode() outside RocksDB. What do you think ?

[mdlugajczyk]

@IslamAbdelRahman - sure that makes sense. I'll modify my change to just add a new subcode. Thanks for reviewing the pull request and your feedback.

[facebook-github-bot]

@mdlugajczyk updated the pull request - view changes - changes since last import

[facebook-github-bot]

@mdlugajczyk updated the pull request - view changes - changes since last import

[mdlugajczyk]

@IslamAbdelRahman - apologies for the delay in getting back to you. I've updated the code to just add a new subcode and pass it to the IOError constructor when the errno is ESTALE. Does it look better now?

[mdlugajczyk]

I've just realised that the current version of the code would lose the error message, would it be ok if I changed the IOError factory function to take subcode & optional error message? Something like

static Status IOError(Subcode subcode = kNone, const Slice& msg = Slice()) { return Status(kIOError, subcode, msg); }

and changing the private constructor for the Status class to:

Status::Status(Code _code, Subcode _subcode = kNone, const Slice& _msg = Slice())

[IslamAbdelRahman]

@mdlugajczyk .. the current implementation have Status::subcode() function that will return the code to you, Should not that be good enough for your application ?

[mdlugajczyk]

@IslamAbdelRahman - the subcode will be sufficient to handle this situation, however the Status::ToString() function will be a generic "stale file error". Currently the error message contains the file name for which the error happened, which is useful for logging and later analysis of the problem.

Currently one can get detailed error message if the ESTALE happens, but no easy way to detect it programmatically. My change makes the the detection possible but loses the error message.

Since all the information is available to rocksdb (errno & which file experienced the problem), I think it make sense to expose it to allow detailed logging & special handling of this error in the user code.

[facebook-github-bot]

@IslamAbdelRahman has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.