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

storage: Resolve intents when encountering an aborted txn #5946

Merged
merged 1 commit into from
Apr 9, 2016

Conversation

kkaneda
Copy link
Contributor

@kkaneda kkaneda commented Apr 8, 2016

This is a follow-up of #5885. Resolve local intents and update the transaction record when we attempt to roll back the transaction and find that it has already been aborted.

Define helper method 1resolveExplicitIntents` and call it.


This change is Reviewable

@bdarnell
Copy link
Contributor

bdarnell commented Apr 8, 2016

LGTM


Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


storage/replica_command.go, line 571 [r1] (raw file):
Remove this log line


storage/replica_command.go, line 589 [r1] (raw file):
Remove this log line


Comments from Reviewable

This is a follow-up of #5885. Resolve local intents and update the
transaction record when we attempt to roll back the transaction and
find that it has already been aborted.

Define helper method resolveExplicitIntents and call it.
@kkaneda kkaneda force-pushed the kaneda/resolve_intent_from_rollback branch from 7c9e044 to 53644ae Compare April 9, 2016 00:34
@kkaneda
Copy link
Contributor Author

kkaneda commented Apr 9, 2016

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.


storage/replica_command.go, line 571 [r1] (raw file):
Ah sorry.. fixed.


storage/replica_command.go, line 589 [r1] (raw file):
Done.


Comments from Reviewable

@kkaneda kkaneda merged commit 4ac7707 into master Apr 9, 2016
@kkaneda kkaneda deleted the kaneda/resolve_intent_from_rollback branch April 9, 2016 01:23
}
}

// Persist the transaction record with updated status (& possibly timestamp).
Copy link
Member

Choose a reason for hiding this comment

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

It's weird that everything that happens from here on down happens inside of this helper method. Auto-gc'ing can easily move outside. Writing an updated txn entry can maybe stay, but the method name needs to indicate that this is happening (and nobody can write another entry in EndTransaction after this one unless it reads the entry from disk first).

@tbg
Copy link
Member

tbg commented Apr 11, 2016

Had some late comments - sorry for not looking earlier.

@kkaneda
Copy link
Contributor Author

kkaneda commented Apr 11, 2016

Sure! Will send another PR to address the comment.

@tbg
Copy link
Member

tbg commented Apr 11, 2016

Thanks Kenji, appreciate it.

On Mon, Apr 11, 2016 at 11:00 AM Kenji Kaneda notifications@github.com
wrote:

Sure! Will send another PR to address the comment.


You are receiving this because you commented.

Reply to this email directly or view it on GitHub
#5946 (comment)

-- Tobias

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.

3 participants