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

Changed background_rollback to return true/false depending on if #212

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

Conversation

sjaakola
Copy link
Contributor

@sjaakola sjaakola commented Apr 4, 2023

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.

@sjaakola sjaakola requested a review from temeo April 4, 2023 10:15
@@ -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;
Copy link
Contributor

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.

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);
Copy link
Contributor

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.

Copy link
Contributor

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;

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 ?

Copy link
Contributor

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.

@sjaakola sjaakola requested a review from temeo April 11, 2023 18:15
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.
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.

None yet

3 participants