-
Notifications
You must be signed in to change notification settings - Fork 13
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
Changed background_rollback to return true/false depending on if #212
base: master
Are you sure you want to change the base?
Conversation
@@ -86,7 +86,7 @@ namespace wsrep | |||
/** | |||
* Perform a background rollback for a transaction. | |||
*/ | |||
virtual void background_rollback(wsrep::client_state&) = 0; | |||
virtual bool background_rollback(wsrep::client_state&) = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document the meaning of return value in comment above.
src/transaction.cpp
Outdated
server_service_.background_rollback(client_state_); | ||
/* if background rollback is skipped, reset rollbacker activity */ | ||
if (server_service_.background_rollback(client_state_)) | ||
client_state_.set_rollbacker_active(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling set_rollbacker_active()
should be done with lock held to avoid races.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it is possible that the client session has reached client_state::do_wait_rollback_complete_and_acquire_ownership()
while the mutex was unlocked above. In order to wake up the client, the code should also notify the client_state
via client_state_.cond_
to wake up the thread which hosts the client session.
background_rollback(wsrep::client_state& client_state) WSREP_OVERRIDE | ||
{ | ||
client_state.before_rollback(); | ||
client_state.after_rollback(); | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I do not understand but when this method can return true ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for unit testing, so it does not necessarily implement all the same behavior as real application.
background rollback did happen. Background rollback should be skipped if the aborting happens due to KILL command issued by user. Some KILL signals, like KILL CONECTION, wake up the victim too early so that background rollback could happen in parallel with the victim waking up and continuing execution.
background rollback did happen.
Background rollback should be skipped if the aborting happens due to KILL command issued by user. Some KILL signals, like KILL CONNECTION, wake up the victim too early so that background rollback could happen in parallel with the victim waking up and continuing execution.