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

Fixed potential data loss in case of ZK errors [#CLICKHOUSE-3916] #2939

Merged

Conversation

alexey-milovidov
Copy link
Member

No description provided.

@alexey-milovidov alexey-milovidov requested a review from ztlpn August 24, 2018 08:58
/** If the connection is lost, and we do not know if the changes were applied, we can not delete the local part
* if the changes were applied, the inserted block appeared in `/blocks/`, and it can not be inserted again.
*
* NOTE that in contrast to original libzookeeper, in our library we may also get ZSESSIONEXPIRED in this case.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok, but because we can't distinguish requests that have failed for sure and those that have probably failed, this branch can execute in more cases than needed.

For the original library only the ZCONNECTIONLOSS and ZOPERATIONTIMEOUT error codes signified that the status of the operation is unknown. Other errors meant that the operation didn't take place. It makes sense to distinguish these cases in our library too: e.g. when we don't send the request because the session has already expired, then we are sure that the operation didn't take place and can safely throw out the new part.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I also had the same thoughts,
but it is resolved by the fact that we check for session expiration (and also allocating block number) not long before and the number of false positive cases (when the operation is actually failed) will be insignificant comparing to previous implementation.

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

Successfully merging this pull request may close these issues.

2 participants