-
Notifications
You must be signed in to change notification settings - Fork 553
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
Introduced cluster::vcluster_id
type underlied by XID
#16538
Introduced cluster::vcluster_id
type underlied by XID
#16538
Conversation
3fda493
to
5ad109c
Compare
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.
const char* what() const noexcept final { return _msg.c_str(); } | ||
|
||
private: | ||
ss::sstring _msg; |
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.
if the code points are static, i.e.: throw invalid_xid("foo-bar-baz")
then you could simply take a string_view
maybe a invalid_xid_view
that takes in a stringview rather than a copy w/ allocation.
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.
in this case the message is formatted when instantiating the exception object.
* | ||
* Based on (https://github.com/rs/xid) | ||
*/ | ||
class xid { |
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.
i wonder if all namespace-topic-partitions should just be xids now ? maybe the flywheight pattern travis had in mind is better for partition ids (ints are cheaper)
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.
this is good idea, it would require a lot of changes, we have numeric ntps planned.
/ci-repeat 1 |
ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45184#018dca92-c0a4-484b-b2a8-4475625906e9 |
5ad109c
to
c11216d
Compare
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 i think there i just some missing coverage in the test.
b32_alphabet[((id._data[2] >> 4) & mask) | ((id._data[1] << 4) & mask)], | ||
b32_alphabet[id._data[3] >> 7 | ((id._data[2] << 1) & mask)], | ||
b32_alphabet[(id._data[3] >> 2) & mask], | ||
b32_alphabet[id._data[4] >> 5 | ((id._data[3] << 3) & mask)], |
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.
Is this producing the same format as https://github.com/rs/xid/blob/master/id.go#L170 ?
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.
yes
c11216d
to
4b45064
Compare
Signed-off-by: Michal Maslanka <michal@redpanda.com>
4b45064
to
488476a
Compare
Signed-off-by: Michal Maslanka <michal@redpanda.com>
Defined a type representing a virtual cluster label. The `vcluster_id` is going to be used to identify partitions and connections and enrich them with virtual cluster context. Signed-off-by: Michal Maslanka <michal@redpanda.com>
488476a
to
8cf25a3
Compare
Defined a type representing a virtual cluster label. The
vcluster_id
is going to be used to identify partitions and connections and enrich them with virtual cluster context.Virtual cluster id is underlaid by XID type as defined in https://github.com/rs/xid.
Backports Required
Release Notes