1PC transactions #3364
Merged
1PC transactions #3364
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
|
|
|
I added a commit which fixes the flaky test. |
the test's success was dependent on application order across multiple nodes and now influences this order appropriately.
|
|
||
| for _, test := range []struct { | ||
| put1, put2, et roachpb.Key | ||
| // -1 is used as separator between requests for simplicity. |
There was a problem hiding this comment.
Unclear what this is referring to. Stale comment?
|
LGTM |
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.
name old time/op new time/op delta SingleRoundtripTxnWithLatency_0-8 2.42ms ± 0% 1.65ms ± 0% ~ (p=1.000 n=1+1) SingleRoundtripTxnWithLatency_10-8 25.3ms ± 0% 13.2ms ± 0% ~ (p=1.000 n=1+1) Both changes are not statistically relevant due to the high cost of a single operation, but the second number is actually consistent (the first will fluctuate somewhat). Their value is mostly in showing the benefit of 1pc transactions end-to-end from the KV client. before: PASS BenchmarkSingleRoundtripTxnWithLatency_0-8 20000 2418298 ns/op BenchmarkSingleRoundtripTxnWithLatency_10-8 1000 25316220 ns/op ok github.com/cockroachdb/cockroach/kv 89.027s PASS after: BenchmarkSingleRoundtripTxnWithLatency_0-8 10000 1647978 ns/op BenchmarkSingleRoundtripTxnWithLatency_10-8 2000 13190856 ns/op ok github.com/cockroachdb/cockroach/kv 44.214s
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.
resuscitation of #2818.
This change enables 1PC txns for transactions 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 #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.
Instead, I reused a
kvbenchmark which runs a 1pc transaction. Withoutactual latency however, the effect of this change is minimal. I added a way
to simulate latency (by means of
time.Sleep) toTestClusterand benchmarkedusing that. The results are, well, not surprising. See second commit.