etcdutl: use map to count unique user keys in snapshot status #19344
Conversation
|
Hi @jxustc. 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. |
|
Thanks for the PR, but I am afraid that the fix isn't accurate. When a key has already been removed from user perspective, but it is still in the db file with tombstone flag set. We shouldn't count such keys. |
Thanks for the feedback. I hadn't considered the tombstone case. I will update the implementation to exclude deleted keys. |
jxustc
force-pushed
the
fix/etcdutl-key-count
branch
from
e67feac to
775af72
Compare
etcdutl/snapshot/v3_snapshot.go
Outdated
| key := string(kv.Key) | ||
| // Version == 0 indicates logical deletion. | ||
| // refer to https://etcd.io/docs/v3.5/learning/data_model/ | ||
| isActive := kv.Version > 0 | ||
| if curr, exists := keyStates[key]; !exists || rev.Main > curr.rev { | ||
| keyStates[key] = struct { | ||
| rev int64 | ||
| active bool | ||
| }{rev.Main, isActive} |
There was a problem hiding this comment.
- Pls use
isTombstoneto check whether it's a tombstone. We can expose the function.etcd/server/storage/mvcc/revision.go
Line 120 in c6b9e9e
- No need to compare revision. They are guaranteed to be increasing.
jxustc
force-pushed
the
fix/etcdutl-key-count
branch
3 times, most recently
from
122d5e1 to
049178d
Compare
etcdutl/snapshot/v3_snapshot_test.go
Outdated
| @@ -118,6 +118,106 @@ func TestSnapshotStatusNegativeRevisionSub(t *testing.T) { | |||
| require.ErrorContains(t, err, "negative revision") | |||
| } | |||
|
|
|||
| // TestSnapshotStatusDuplicateKeys tests if TotalKey in status remains unchanged when inserting the same key multiple times | |||
| func TestSnapshotStatusDuplicateKeys(t *testing.T) { | |||
There was a problem hiding this comment.
All the test cases should be able to be consolidated into one table-driven sub tests. But not a big problem.
|
/ok-to-test |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
... and 32 files with indirect coverage changes @@ Coverage Diff @@
## main #19344 +/- ##
==========================================
+ Coverage 68.84% 68.93% +0.09%
==========================================
Files 420 420
Lines 35716 35731 +15
==========================================
+ Hits 24587 24630 +43
+ Misses 9695 9680 -15
+ Partials 1434 1421 -13 Continue to review full report in Codecov by Sentry.
|
|
Thanks for the review. You make a good point about table-driven tests. I'm happy to refactor them. I also noticed some e2e test failures due to the changed TotalKey semantics - I'll fix those as well. |
The new implementation: - Uses map to track unique keys for accurate counting - Excludes internal built-in keys from total count - Improves code maintainability Although this approach uses additional memory for the map, the trade-off is acceptable since: - Status() is not in hot path - Correctness takes priority over performance optimization - Simpler code is easier to maintain Fixes etcd-io#19253 Signed-off-by: Xiang Ji <[email protected]>
jxustc
force-pushed
the
fix/etcdutl-key-count
branch
from
f7751dd to
2d37700
Compare
There was a problem hiding this comment.
LGTM
Please also update 3.6 changelog https://github.com/etcd-io/etcd/blob/main/CHANGELOG/CHANGELOG-3.6.md#etcdutl-v3
thx
It could be in a in a separate PR |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, fuweid, jxustc 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 |
Done in #19361 |
Thx |
The new implementation:
Although this approach uses additional memory for the map, the trade-off is acceptable since:
Fixes #19253
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.