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

Recovery from connection loss or server-side errors #144

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Recovery from connection loss or server-side errors #144

wants to merge 3 commits into from

Conversation

sibiryakov
Copy link

@sibiryakov sibiryakov commented Nov 8, 2016

The idea is to fall into infinite cycle until connection is re-established and command retried and executed successfully. For write operations retrying isn't performed and exceptions propagated, but connection is refreshed to be successful during the subsequent calls.

@sibiryakov
Copy link
Author

any thoughts @wbolster ?

@sibiryakov
Copy link
Author

@wbolster please have a look at this.

@wbolster
Copy link
Member

hi, thanks for your contribution, and sorry for not looking at this earlier. (other priorities in life.)

in general, i like the idea behind this pr, but i have some doubts about this implementation. some remarks:

  • in general subclasses make me sad...

    • this code uses a TClient subclass. that class is not from happybase but from thrifty, and this code pokes into its internals to override some behaviour. i can't say i like that.

    • there must be a better way. i strongly favour composition over inheritance, and i think that is the way to go here: encapsulate the thriftpy.TClient and add behaviour in our own class, then delegate to the encapsulated TClient to do the actual work.

  • this behaviour should be optional configurable at the Connection level.

what do you think?

@wbolster
Copy link
Member

few other things:

  • there are no tests

  • what about using a lib like retrying?

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

Successfully merging this pull request may close these issues.

2 participants