allow 1PC txns #2818
Closed
allow 1PC txns #2818
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
add a new flag that simply keeps EndTransaction in its current chunk when set.
in preparation for allowing 1pc txns.
ba is not a pointer any more.
| if ba.Txn == nil && ba.ReadConsistency != roachpb.INCONSISTENT { | ||
| return nil, roachpb.NewError(&roachpb.OpRequiresTxnError{}), false | ||
| } | ||
| // If the request is more than but ends with EndTransaction, we |
There was a problem hiding this comment.
Comment is missing some words
|
My suggestion for clarity is a more substantial refactoring. We split batches for two reasons, by type (formerly in For each request in the batch:
|
|
I'll give it a try. Agreed that the current code isn't clean enough. The refactor will be substantial, but I think it's overdue anyway. |
|
don't see this making progress soon, so will close this to declutter. |
Merged
tbg
added a commit
to tbg/cockroach
that referenced
this pull request
Dec 9, 2015
resuscitation of cockroachdb#2818. This change enables 1PC txns for transaction contained in a single batch and sent to a single Range. Contrary to the previous incarnation, now the chunk of batch containing EndTransaction is split and re-sent before any other redundant requests have been made. More improvement is possible: If the EndTransaction request were to end up in the last Batch sent over the course of the multi-range request anyway, it would not need to be split up into its own chunk. There's no way of knowing that until the last range descriptor has been looked up though, at which point in time we've already sent requests, complicating the code. Hence, this optimization (which amounts to @bdarnell's suggestion in cockroachdb#2818) has been omitted for now due to the amount of code changes required. Unfortunately, the improvements here don't directly reflect in SQL benchmarks because of the way the Txn is used there. We need to optimize the SQL server to batch up as many commands as it can (basically until it reads data) and commit in the same round-trip if possible. Currently the Txn object is used deep down in the individual operations (for example `(*planner).Insert()`) so this isn't trivial. I will file an issue.
tbg
added a commit
to tbg/cockroach
that referenced
this pull request
Dec 9, 2015
resuscitation of cockroachdb#2818. This change enables 1PC txns for transaction contained in a single batch and sent to a single Range. Contrary to the previous incarnation, now the chunk of batch containing EndTransaction is split and re-sent before any other redundant requests have been made. More improvement is possible: If the EndTransaction request were to end up in the last Batch sent over the course of the multi-range request anyway, it would not need to be split up into its own chunk. There's no way of knowing that until the last range descriptor has been looked up though, at which point in time we've already sent requests, complicating the code. Hence, this optimization (which amounts to @bdarnell's suggestion in cockroachdb#2818) has been omitted for now due to the amount of code changes required. Unfortunately, the improvements here don't directly reflect in SQL benchmarks because of the way the Txn is used there. We need to optimize the SQL server to batch up as many commands as it can (basically until it reads data) and commit in the same round-trip if possible. Currently the Txn object is used deep down in the individual operations (for example `(*planner).Insert()`) so this isn't trivial. I will file an issue.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change enables 1PC txns for transaction contained in a single batch and sent to a single
Range.I'm going to add some tests for this, but the meat is ready for review.
The interplay introduced around
sendChunkin DistSender and is somewhat weird and passes back an extra boolean (avoiding the need for a new proto error). I'm open to suggestions for making that prettier.