Refactor zookeeper error handling #595
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This one should be handled also |
|
This is weird reaction of Kazoo to "SessionExpiration" situation. We can indeed isolate the must "noisy" calls here and make them more resistant against these exceptions, in this case it is: @mithunbharadwaj pls task for this In general, we need to be careful not to hide too much exceptions from Zookeeper otherwise we will start observing a "magically strange" behaviour of the library. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi, @mithunbharadwaj , @ateska
I would like to discuss error handling in zookeeper code.
Catching NoNodeError in the library provider is useless, becuase this one is already caught in the wrapper.
We experience uncaught kazoo errors in every microservice when zookeeper disconnects (see sentry for more detail). I think it originates from this point in the zookeeper library provider - we resigned to handle the errors. However, those should be either propagated further as a specific Library Error (so it can be caught in the microservice) or all the errors IN READ mode (i.e. in the library) should be silent and return None.
Right now, we mix both approaches - NoNode Errors are ignored (returning None) and other errors are not caught at all. That means that in my microservice, I always have to check for None and also do try/except for the rest of kazoo errors (which we typically don't do).
On the other hand, I suggest to propagate all kazoo errors from WRITE operations in the wrapper to the microservice, so I can catch them and decide specifically what I want to do with them. (write operations are not in the library provider.) Ignoring NoNodeError in write operations does not seem very convenient to me. Especially, when I need to handle other errors, anyway. https://github.com/TeskaLabs/asab/blob/master/asab/zookeeper/wrapper.py#L78
I'm afraid I missed some use cases that won't fit my suggestions. However, I'd like to agree on some "ideal" approach how to read from ZK, so we diminish the kazoo errors in sentry.