Skip to content
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.

Sign up for GitHub

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

Jump to bottom

etcdutl: use map to count unique user keys in snapshot status #19344

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

etcdutl: use map to count unique user keys in snapshot status #19344

wants to merge 2 commits into from
+119 −1

Conversation

jxustc
Copy link

@jxustc jxustc commented Feb 5, 2025

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 #19253

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

@jxustc
etcdutl: use map to count unique user keys in snapshot status
e590b69 
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]>
@k8s-ci-robot
Copy link

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jxustc
Once this PR has been reviewed and has the lgtm label, please assign jmhbnz for approval. For more information see the Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ahrtr
Copy link
Member

ahrtr commented Feb 5, 2025

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.

Refer to https://etcd.io/docs/v3.5/learning/data_model/

@jxustc
Copy link
Author

jxustc commented Feb 6, 2025 •
edited
Loading

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.

Refer to https://etcd.io/docs/v3.5/learning/data_model/

Thanks for the feedback. I hadn't considered the tombstone case. I will update the implementation to exclude deleted keys.

@k8s-ci-robot k8s-ci-robot added size/L and removed size/S labels Feb 6, 2025
@jxustc
etcdutl: fix key count accuracy in snapshot status
775af72 
This commit refines the key existence determination strategy to strictly
align with etcd's data model.

Fixes etcd-io#19253

Signed-off-by: Xiang Ji <[email protected]>
@jxustc jxustc force-pushed the fix/etcdutl-key-count branch from e67feac to 775af72 Compare February 6, 2025 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
area/etcdutl needs-ok-to-test size/L
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

etcdutl Snapshot Status: TotalKey Calculation Inquiry
3 participants
@jxustc @k8s-ci-robot @ahrtr