-
Notifications
You must be signed in to change notification settings - Fork 710
db: drop unused txs list #969
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
Conversation
Seems like it was more useful before 263e75d, but now it's only used for statistics which can easily be managed in a different way. I see no other valid purposes for this list, a reference can have some value for GC, but if DB user loses a reference to transaction that is not closed there is not much DB can do. This improves ConcurrentView test from workers samples min avg 50% 80% 90% max 1 10 49.323µs 974.287µs 1.068978ms 1.112882ms 1.131938ms 1.131938ms 10 100 32.592µs 685.315µs 980.5µs 1.125385ms 1.137678ms 1.169789ms 100 1000 31.49µs 219.084µs 77.427µs 353.651µs 656.916µs 1.785808ms 1000 10000 30.668µs 1.639366ms 99.128µs 3.086665ms 5.031354ms 16.315849ms 10000 100000 30.818µs 40.893475ms 36.963667ms 78.650583ms 111.553136ms 302.412177ms to workers samples min avg 50% 80% 90% max 1 10 78.358µs 964.847µs 1.059159ms 1.073256ms 1.07551ms 1.07551ms 10 100 32.802µs 304.922µs 80.924µs 674.54µs 1.069298ms 1.220625ms 100 1000 30.758µs 304.541µs 64.192µs 397.094µs 1.101991ms 2.183302ms 1000 10000 30.558µs 1.05711ms 92.426µs 2.111896ms 3.317894ms 11.790014ms 10000 100000 30.548µs 10.98898ms 90.742µs 21.740659ms 33.020076ms 135.33094ms Signed-off-by: Roman Khimov <[email protected]>
|
Hi @roman-khimov. Thanks for your PR. I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
|
I thought @tjungblu wanted to proceed on refactoring the I recall we have found a bug, which was the motivation for this refactoring iiuc |
Yes, but it doesn't conflict with this cleanup. |
|
ok, then if it was used only for stats, we can remove it LGTM 👍🏽 |
ahrtr
left a comment
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.
LGTM & thx
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, roman-khimov The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Opss, I didn't realize that this PR is against release-1.4. We should avoid making such change in stable release. We should only make the change on main branch instead. Action: revert the change in release-1.4, and deliver a PR on main instead. cc @roman-khimov @Elbehery |
|
That was one of the questions of #967. I'll rebase to main and open a new PR. |
|
I will revert it now 👍🏽 |
This reverts commit f33bb13, reversing changes made to cc41cbb. Signed-off-by: Mustafa Elbehery <[email protected]>
…-list [release-1.4] Revert "Merge pull request #969 from nspcc-dev/drop-unused-txs"
A part of from #967, the test itself isn't added here, it's rather trivial and likely not the best way to check these problems, but it was sufficient for the purpose.
Seems like it was more useful before 263e75d, but now it's only used for statistics which can easily be managed in a different way. I see no other valid purposes for this list, a reference can have some value for GC, but if DB user loses a reference to transaction that is not closed there is not much DB can do. This improves ConcurrentView test from
workers samples min avg 50% 80% 90% max
1 10 49.323µs 974.287µs 1.068978ms 1.112882ms 1.131938ms 1.131938ms
10 100 32.592µs 685.315µs 980.5µs 1.125385ms 1.137678ms 1.169789ms
100 1000 31.49µs 219.084µs 77.427µs 353.651µs 656.916µs 1.785808ms
1000 10000 30.668µs 1.639366ms 99.128µs 3.086665ms 5.031354ms 16.315849ms
10000 100000 30.818µs 40.893475ms 36.963667ms 78.650583ms 111.553136ms 302.412177ms
to
workers samples min avg 50% 80% 90% max
1 10 78.358µs 964.847µs 1.059159ms 1.073256ms 1.07551ms 1.07551ms
10 100 32.802µs 304.922µs 80.924µs 674.54µs 1.069298ms 1.220625ms
100 1000 30.758µs 304.541µs 64.192µs 397.094µs 1.101991ms 2.183302ms
1000 10000 30.558µs 1.05711ms 92.426µs 2.111896ms 3.317894ms 11.790014ms
10000 100000 30.548µs 10.98898ms 90.742µs 21.740659ms 33.020076ms 135.33094ms