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.

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

Fix range_end calculation #27

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

Fix range_end calculation #27

wants to merge 2 commits into from

Conversation

venu
Copy link
Contributor

@venu venu commented May 8, 2024

Issue

We are adding +1 to the last byte excluding /, because of this, we are not receiving events. Escpecially when you set the persmission based on the path like below.

role permission

etcdctl role grant-permission clientx --prefix=true readwrite app_data/clients/clientx2/

watch prefix for app_data/clients/clientx2/ becomes b'app_data/clients/clientx3/' but it supposed to be b'app_data/clients/clientx30' (As slash is part of the key, we shall add +1 to the slash)

Fix

As per etcd3 spec https://etcd.io/docs/v3.3/learning/api/#:~:text=revision%20was%20committed.-,Key%20ranges,-The%20etcd3%20data

By convention, ranges for a request are denoted by the fields key and range_end. The key field is the first key of the range and should be non-empty. The range_end is the key following the last key of the range. If range_end is not given or empty, the range is defined to contain only the key argument. If range_end is key plus one (e.g., “aa”+1 == “ab”, “a\xff”+1 == “b”), then the range represents all keys prefixed with key. If both key and range_end are ‘\0’, then range represents all keys. If range_end is ‘\0’, the range is all keys greater than or equal to the key argument.

Some implementations

https://github.com/etcd-io/etcd/blob/main/client/v3/op.go#L376
https://github.com/gaopeiliang/aioetcd3/blob/4709a2a412cc81d47a8a262e569b4cd62a7659d4/aioetcd3/utils.py#L14

@stalkerg
Copy link

stalkerg commented May 8, 2024

https://github.com/etcd-io/etcd/blob/main/client/v3/op.go#L376 -- it's how etcdctl do it.

@@ -362,10 +362,7 @@ def get_prefix(
encoding='utf-8',
):
encoded_key = key.encode(encoding)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need this encode here we already do it inside new function. Same for other cases.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or new function shouldn't worry about conversion and should consume only bytes

@venu venu marked this pull request as ready for review May 9, 2024 05:52
Copy link

@stalkerg stalkerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stalkerg
Copy link

stalkerg commented May 9, 2024

@kyujin-cho please review this PR.

@stalkerg
Copy link

@kyujin-cho sorry, but ping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants