-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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: strip Transaction proto from individual Response messages #42139
Merged
craig
merged 1 commit into
cockroachdb:master
from
nvanbenschoten:nvanbenschoten/dontReplyTxn
Nov 4, 2019
Merged
storage: strip Transaction proto from individual Response messages #42139
craig
merged 1 commit into
cockroachdb:master
from
nvanbenschoten:nvanbenschoten/dontReplyTxn
Nov 4, 2019
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
This commit ensures that we strip Transaction protos from individual Response message headers before they are put on this wire. These Transaction objects are already used to update the Transaction proto on the BatchResponse header, so there's no reason to include these in individual Responses as well. Before this change, the following Response types contained the extra information: - `EndTransactionResponse` - `BeginTransactionResponse` - `HeartbeatTxnResponse` - `QueryIntentResponse` Release note (performance improvement): Individual response messages in a response batch no longer each contain information about transaction state changes.
tbg
approved these changes
Nov 4, 2019
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.
Reviewed 4 of 4 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained
bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Nov 4, 2019
42139: storage: strip Transaction proto from individual Response messages r=nvanbenschoten a=nvanbenschoten This commit ensures that we strip `Transaction` protos from individual `Response` message headers before they are put on this wire. These `Transaction` objects are already used to update the `Transaction` proto on the `BatchResponse` header, so there's no reason to include these in individual `Responses` as well. Before this change, the following `Response` types contained the extra information: - `EndTransactionResponse` - `BeginTransactionResponse` - `HeartbeatTxnResponse` - `QueryIntentResponse` This reduces total network traffic in a cluster running non-1PC transactions by up to **3.00%**. This was determined through the following experiment: ``` Cluster: 3x n1-standard-16 VMs Load gen: 1 per node to avoid leaseholder effects (33% of KV RPCs are local) on same hosts as crdb nodes to avoid client<->gateway network traffic Setup: `workload init indexes --secondary-indexes=1` Workload: (on each VM) `workload run indexes --secondary-indexes=1 --payload=1 --concurrency=60 --max-rate=2000 --ramp=1m --duration=3m` --max-rate ensures that both trials run the exact same number of txns ``` The workload was run first without this change (on the left) and next with this change (on the right). The table was dropped in between. <img width="989" alt="Screen Shot 2019-11-03 at 1 23 57 AM" src="https://user-images.githubusercontent.com/5438456/68080878-bb627300-fdda-11e9-9e0c-9da165bec60c.png"> <img width="1245" alt="Screen Shot 2019-11-03 at 1 24 14 AM" src="https://user-images.githubusercontent.com/5438456/68080879-bb627300-fdda-11e9-8745-b9211b9de3c3.png"> <img width="988" alt="Screen Shot 2019-11-03 at 1 24 21 AM" src="https://user-images.githubusercontent.com/5438456/68080880-bb627300-fdda-11e9-89b0-185985f34095.png"> We see from the aggregated results that without this change, the cluster transmitted **7.01 GB** on the network over the course of the benchmark. With this change, the cluster transmitted **6.80 GB** on the network over the course of the benchmark. This indicates a reduction in network traffic by **3.00%**. We can do out the math here to see whether the results of this experiment add up. The change avoids transmitting 2 `Transaction` protos per transaction over the network. These were in the responses of `EndTxn(STAGING)` and of `EndTxn(COMMITTED)` (see parallel commits). In the cluster, we were running exactly 6,000 transactions per second. So this change avoids 12,000 `Transaction` protos per second. Assuming these protos have an average size of 116 bytes (the encoded size of `nonZeroTxn` in `roachpb/data_test.go`), we would expect this to result in a reduction in network throughput of 1.4 MB/s. This is roughly what we see in the third graph. Release note (performance improvement): Individual response messages in a response batch no longer each contain information about transaction state changes. Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Build succeeded |
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 commit ensures that we strip
Transaction
protos from individualResponse
message headers before they are put on this wire. TheseTransaction
objects are already used to update theTransaction
proto on theBatchResponse
header, so there's no reason to include these in individualResponses
as well.Before this change, the following
Response
types contained the extra information:EndTransactionResponse
BeginTransactionResponse
HeartbeatTxnResponse
QueryIntentResponse
This reduces total network traffic in a cluster running non-1PC transactions by up to 3.00%. This was determined through the following experiment:
The workload was run first without this change (on the left) and next with this change (on the right). The table was dropped in between.
We see from the aggregated results that without this change, the cluster transmitted 7.01 GB on the network over the course of the benchmark. With this change, the cluster transmitted 6.80 GB on the network over the course of the benchmark. This indicates a reduction in network traffic by 3.00%.
We can do out the math here to see whether the results of this experiment add up. The change avoids transmitting 2
Transaction
protos per transaction over the network. These were in the responses ofEndTxn(STAGING)
and ofEndTxn(COMMITTED)
(see parallel commits). In the cluster, we were running exactly 6,000 transactions per second. So this change avoids 12,000Transaction
protos per second. Assuming these protos have an average size of 116 bytes (the encoded size ofnonZeroTxn
inroachpb/data_test.go
), we would expect this to result in a reduction in network throughput of 1.4 MB/s. This is roughly what we see in the third graph.Release note (performance improvement): Individual response messages in a response batch no longer each contain information about transaction state changes.