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

Uuid support in mock cluster #4591

Merged
merged 2 commits into from
Feb 13, 2024
Merged

Conversation

emasab
Copy link
Contributor

@emasab emasab commented Jan 24, 2024

No description provided.

@emasab emasab force-pushed the dev_mock_cluster_topic_id_support branch from 25b5960 to 4fa605c Compare January 25, 2024 12:06
Comment on lines +5093 to +5105
for (i = 0; i < 16; i += 2) {
uint16_t rand_uint16 = (uint16_t)rd_jitter(0, INT16_MAX - 1);
/* No need to convert endianess here because it's still only
* a random value. */
rand_values_app = (unsigned char *)&rand_uint16;
rand_values_bytes[i] |= rand_values_app[0];
rand_values_bytes[i + 1] |= rand_values_app[1];
}

rand_values_bytes[6] &= 0x0f; /* clear version */
rand_values_bytes[6] |= 0x40; /* version 4 */
rand_values_bytes[8] &= 0x3f; /* clear variant */
rand_values_bytes[8] |= 0x80; /* IETF variant */
Copy link
Member

Choose a reason for hiding this comment

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

I think we should move this to rdrand.h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It generates a Uuid, all the code for a Uuid is here

Copy link
Contributor Author

@emasab emasab Feb 7, 2024

Choose a reason for hiding this comment

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

Maybe I can move it to a rdkafka_uuid.h and rdkafka_uuid.c

Copy link
Member

Choose a reason for hiding this comment

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

I was talking about the generic part related to random bytes generation here.

Copy link
Member

Choose a reason for hiding this comment

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

Regarding rdkafka_uuid.h and rdkafka_uuid.c, we can have them but don't do it for now. Let's do it later when we we won't have much conflicts with other ongoing dev work.

Copy link
Contributor Author

@emasab emasab Feb 7, 2024

Choose a reason for hiding this comment

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

I think we should move this to rdrand.h

here the random value is generated in rd_jitter that is already in rdrand.h the rest is code related to Uuid

src/rdkafka.c Show resolved Hide resolved
Comment on lines 1059 to 1061
RD_EXPORT rd_kafka_Uuid_t *rd_kafka_Uuid_random();

RD_EXPORT const char *rd_kafka_Uuid_str(const rd_kafka_Uuid_t *uuid);
Copy link
Member

Choose a reason for hiding this comment

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

These can be in proto.h and the definitions should be in proto.c as everything internal related to Uuid is present there.

src/rdkafka.c Outdated
*
* @remark Must be freed after use using rd_kafka_Uuid_destroy().
*/
rd_kafka_Uuid_t *rd_kafka_Uuid_random() {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we returning a pointer instead of a value here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, better to return the struct, its usage is internal only

Comment on lines +5093 to +5105
for (i = 0; i < 16; i += 2) {
uint16_t rand_uint16 = (uint16_t)rd_jitter(0, INT16_MAX - 1);
/* No need to convert endianess here because it's still only
* a random value. */
rand_values_app = (unsigned char *)&rand_uint16;
rand_values_bytes[i] |= rand_values_app[0];
rand_values_bytes[i + 1] |= rand_values_app[1];
}

rand_values_bytes[6] &= 0x0f; /* clear version */
rand_values_bytes[6] |= 0x40; /* version 4 */
rand_values_bytes[8] &= 0x3f; /* clear variant */
rand_values_bytes[8] |= 0x80; /* IETF variant */
Copy link
Member

Choose a reason for hiding this comment

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

I was talking about the generic part related to random bytes generation here.

*
* @remark Must be freed after use using rd_kafka_Uuid_destroy().
*/
rd_kafka_Uuid_t rd_kafka_Uuid_random() {
Copy link
Member

Choose a reason for hiding this comment

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

Move this to rdkafka_proto.h where all the other internal functions related to Uuid are present

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is just a header we cannot move all the implementation there, we can refactor to its own file

*
* @remark Must be freed after use.
*/
const char *rd_kafka_Uuid_str(const rd_kafka_Uuid_t *uuid) {
Copy link
Member

Choose a reason for hiding this comment

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

Move this to rdkafka_proto.h where all the other internal functions related to Uuid are present

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same

Comment on lines +5093 to +5105
for (i = 0; i < 16; i += 2) {
uint16_t rand_uint16 = (uint16_t)rd_jitter(0, INT16_MAX - 1);
/* No need to convert endianess here because it's still only
* a random value. */
rand_values_app = (unsigned char *)&rand_uint16;
rand_values_bytes[i] |= rand_values_app[0];
rand_values_bytes[i + 1] |= rand_values_app[1];
}

rand_values_bytes[6] &= 0x0f; /* clear version */
rand_values_bytes[6] |= 0x40; /* version 4 */
rand_values_bytes[8] &= 0x3f; /* clear variant */
rand_values_bytes[8] |= 0x80; /* IETF variant */
Copy link
Member

Choose a reason for hiding this comment

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

Regarding rdkafka_uuid.h and rdkafka_uuid.c, we can have them but don't do it for now. Let's do it later when we we won't have much conflicts with other ongoing dev work.

@emasab emasab force-pushed the dev_mock_cluster_topic_id_support branch from 0011cf9 to b5fe084 Compare February 9, 2024 12:07
@emasab emasab force-pushed the dev_mock_cluster_topic_id_support branch from b5fe084 to 69c0311 Compare February 9, 2024 13:40
Copy link
Member

@pranavrth pranavrth left a comment

Choose a reason for hiding this comment

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

LGTM!

@emasab emasab merged commit a6d85bd into master Feb 13, 2024
2 checks passed
@emasab emasab deleted the dev_mock_cluster_topic_id_support branch February 13, 2024 14:31
anchitj pushed a commit that referenced this pull request Apr 1, 2024
also adds the possibility to generate pseudo-random Uuids
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