-
Notifications
You must be signed in to change notification settings - Fork 10.2k
do not remove range requests from TXN to ensure all members execute the same validation for TXN #20545
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
base: main
Are you sure you want to change the base?
Conversation
…he same validation for TXN Signed-off-by: hwdef <[email protected]> Co-authored-by: Benjamin Wang <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: hwdef The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 23 files with indirect coverage changes @@ Coverage Diff @@
## main #20545 +/- ##
==========================================
- Coverage 69.11% 69.09% -0.02%
==========================================
Files 420 420
Lines 34776 34763 -13
==========================================
- Hits 24034 24021 -13
- Misses 9342 9345 +3
+ Partials 1400 1397 -3 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
I don't think you tests results match up with my expectations. What's the cluster size in your setup? What command you run to execute benchmarks? Is your benchmark tool connected to all members or just one as by default. |
I follow this document
Are my steps correct? |
Thanks for command, that makes it clear. While This can be done by adding EDIT: added |
Also noticed a strange behavior in benchmark tool. Just specifying endpoints for all members will not change the fact that the benchmark will connect to just the first member. You need to also remember to specify This is very unituitive and should be treated as a bug. cc @ahrtr @fuweid @kishen-v |
@serathius txn-mixedOriginal
Modified
txn-putOriginal
Modify
|
I feel like the results haven't changed much. I don't know why. |
Agree it's bug. From my perspective, |
Yes, I'll create a PR for this. |
The fix is definitely correct, but the concern is the impact on performance (memory usage), but the problem is that it might be hard to verify/test. When there is a large range request in TXN, only the member which the client connects to will have bigger memory usage, after the PR, all members will have bigger memory usage. When there are large number of clients/connections, we can load balance them across multiple members. After this PR, the overall memory usage will increase when there is large range in TXNs. Have you tried the other solution (all members execute the same validation but only the member the client connects to execute the range request) mentioned in #18667 (comment)? The third approach is to document the known issue and leave it as it's for now. I have never seen a real use case to trigger the issue, at least Kubernetes isn't affected currently. |
I haven’t tried the second approach yet, but I plan to. BTW, I think it would be better to first improve the benchmark tests. No matter which solution we end up with, if the benchmark results aren’t convincing, the review process could still be blocked. |
Please raise a separate issue to clarify what's the issue and how to improve it. |
fix: #18667
The code comes from #18667 (comment)
Performance Testing:
txn-mixed
Original
Modified
txn-put
Original
Modified
After the modification, there are some performance differences. I think this is just noise, as the results varied across multiple test runs.
cc @ahrtr @siyuanfoundation