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

Store offset commit metadata when calling rd_kafka_offsets_store #4084

Conversation

mathispesch
Copy link

@mathispesch mathispesch commented Nov 30, 2022

Fixes #3927.

Offset commit metadata can be used to send additional data to the broker when making an offset commit. See this doc from the Java consumer.

Librdkafka currently sends the offset commit metadata when the commit is requested by calling rd_kafka_commit_queue but not when calling rd_kafka_offsets_store to use the auto-commit. This PR fixes that by storing the offset commit metadata when calling rd_kafka_offsets_store.

@mathispesch
Copy link
Author

Hey @edenhill or @ijuma,
Could you please give this PR a look? 🙂

@ijuma
Copy link
Member

ijuma commented Jan 4, 2023

cc @emasab

@milindl milindl requested a review from a team January 5, 2023 11:49
@mathispesch
Copy link
Author

Hey folks,
Have you had the chance to look into this PR?

Copy link
Contributor

@milindl milindl left a comment

Choose a reason for hiding this comment

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

Looks good to me, besides the tiny issue with the test. I'd like @emasab to weigh in.
We should add back the CHANGELOG, but under the next version, (we can use 2.1.0, or just call it vNext or something).

"Retrieving committed offsets to verify committed offset "
"metadata\n");
rd_kafka_topic_partition_list_t *committed_toppar;
committed_toppar = rd_kafka_topic_partition_list_new(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be destroyed at the end of the test, rd_kafka_topic_partition_list_destroy(committed_toppar);

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! Fixed this in 5d37ab1.

@milindl milindl requested a review from emasab January 25, 2023 07:35
@mathispesch mathispesch requested review from milindl and removed request for emasab January 31, 2023 13:20
@milindl milindl requested a review from emasab February 2, 2023 09:11
@emasab emasab changed the base branch from master to feature/store-offsets-metadata February 2, 2023 09:12
@emasab
Copy link
Collaborator

emasab commented Feb 2, 2023

@mathispesch Thanks for the contribution! For security reason currently it's not allowed to run SemaphoreCI on forked PRs. I merge it into an internal branch and open a new PR. If you want to do additional changes before merging to master please open a new PR based on that branch.

@emasab emasab merged commit 5458230 into confluentinc:feature/store-offsets-metadata Feb 2, 2023
@mathispesch mathispesch deleted the store-offsets-metadata branch February 2, 2023 09:56
@mensfeld
Copy link

@mathispesch @emasab should it also store metadata when using rd_kafka_offsets_store and then rd_kafka_commit with null or not? 🤔

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.

rd_kafka_offsets_store does not store and send offset commit metadata
5 participants