Skip to content

Fallback to fetch v12 (#10)#4806

Merged
Emanuele Sabellico (emasab) merged 2 commits intomasterfrom
dev_fix_fetch_fallback
Sep 24, 2024
Merged

Fallback to fetch v12 (#10)#4806
Emanuele Sabellico (emasab) merged 2 commits intomasterfrom
dev_fix_fetch_fallback

Conversation

@anchitj
Copy link
Copy Markdown
Contributor

If any toppars have a zero topic id

How to Reproduce

Set inter.broker.protocol.version to 2.4 with 3.7 and fetch would fail always.

If any toppars have a zero topic id
@anchitj Anchit Jain (anchitj) requested a review from a team as a code owner August 8, 2024 08:23
Comment thread src/rdkafka_fetcher.c Outdated
}

/**
* @brief Check if any toppars have a non-zero topic id.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will change to zero topic id

Comment thread src/rdkafka_fetcher.c Outdated
rkbuf = rd_kafka_buf_new_flexver_request(

/* Fallback to version 12 if topic id is null which can happen if
* inter.broker.protocol.version is old */
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Change old to <2.8

Copy link
Copy Markdown
Contributor

@emasab Emanuele Sabellico (emasab) left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Let's make your changes and my comment and rebase it, then I'm approving

Comment thread src/rdkafka_fetcher.c Outdated
* @brief Check if any toppars have a non-zero topic id.
*
*/
rd_bool_t can_use_topic_id(rd_kafka_broker_t *rkb) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add static, better calling it can_use_topic_ids (as in J)

Suggested change
rd_bool_t can_use_topic_id(rd_kafka_broker_t *rkb) {
static rd_bool_t can_use_topic_ids(rd_kafka_broker_t *rkb) {

@confluent-cla-assistant
Copy link
Copy Markdown

🎉 All Contributor License Agreements have been signed. Ready to merge.
Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM Thanks Anchit

@emasab Emanuele Sabellico (emasab) deleted the dev_fix_fetch_fallback branch September 24, 2024 14:12
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