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

support variable length ZId #1257

Closed
wants to merge 1 commit into from
Closed

Conversation

LizardWizzard
Copy link
Contributor

Involves some workarounds for serde, which is currently does not support
arrays with length expressed as a const generic parameter. This
workaround might be useful in other places if or when we'll want to use
arrays with const generics.

Involves some workarounds for serde, which is currently does not support
arrays with length expressed as a const generic parameter. This
workaround might be useful in other places if or when we'll want to use
arrays with const generics.
@hlinnaka
Copy link
Contributor

What do we need variable length Zids for?

@LizardWizzard
Copy link
Contributor Author

@hlinnaka see #1228 (comment)

@hlinnaka
Copy link
Contributor

What do we need variable length Zids for?

Ok, I see, you want to use a shorter ID for "ZNodeId". I'd suggest just using an int8 for those. And decimal representation would feel more natural too, if they're assigned sequentially rather than by random.

@LizardWizzard
Copy link
Contributor Author

Actually I think 128bit ids are not that big, since we use one per whole machine (#1228 (comment))

Regarding int8 I think it is too small. But anyway, IMO it is more convenient to use one style for all of our ids (timeline, tenant). They are meant to be managed externally by console, so there may be not much sense in random values for all of them then

@arssher
Copy link
Contributor

arssher commented Feb 10, 2022

What do we need variable length Zids for?

Ok, I see, you want to use a shorter ID for "ZNodeId". I'd suggest just using an int8 for those. And decimal representation would feel more natural too, if they're assigned sequentially rather than by random.

Hm, ok. I initially wanted to assign randomly, which has some pros -- centralized issuing is not required and they don't intersect between clusters, but I'm slightly bothered by very long (16 bytes) ids. I agree though random values be better 128 bits, so let's just assign int8 sequentially.

@LizardWizzard
Copy link
Contributor Author

@arssher are we switching for timeline id and tenant id for sequential ones too? Do we really need two flavours of ids? isn't int8 too small for that? If we want to serve thousands of tenants on a single cluster consisting of multiple pageservers/safekeepers that seems to be not enough.

@arssher
Copy link
Contributor

arssher commented Feb 10, 2022

No, only node ids. There is actually some sense in having ids of different flavours -- you can guess the type by looking at it. I'm for that variant mostly due to shortness/readability/being easy to remember. int8 means 8 bytes, not bits, that's postgres terminology, so surely enough.

@LizardWizzard
Copy link
Contributor Author

@arssher If we need to distinguish between entities that ids belong to, lets do that explicitly. We can reserve some of these id bits to some tag value and use it in Debug/Display impls. So id will look something like that: node/abcd12354 or tenant/1234abcd. IMO it is better than some heuristics. Also if our ids are too long, lets shrink them to minimal possible length. e g to have 8 bytes instead of 16

that's postgres terminology,

Good to know, thanks!

@LizardWizzard LizardWizzard deleted the dkr/variable-size-zid branch February 24, 2022 11:04
@chaporgin chaporgin mentioned this pull request May 13, 2022
3 tasks
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.

3 participants