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

Destroy control messages in batch mode #2990

Merged
merged 1 commit into from
Jul 20, 2020

Conversation

pf-qiu
Copy link

@pf-qiu pf-qiu commented Jul 15, 2020

If there are control messages in the topic, rd_kafka_destroy will
hang if only rd_kafka_consume_batch is used, rd_kafka_consume and
rd_kafka_consume_callback don't have this issue.

Release these messages before returning to the application.

@@ -603,7 +603,10 @@ int rd_kafka_q_serve_rkmessages (rd_kafka_q_t *rkq, int timeout_ms,
/* If this is a control messages, don't return
* message to application, only store the offset */
if (unlikely(rd_kafka_op_is_ctrl_msg(rko)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put open-brace on same line as if, like so:

if (unlikely(rd_kafka_op_is_ctrl_msg(rko))) {
 ..

Copy link
Contributor

@edenhill edenhill left a comment

Choose a reason for hiding this comment

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

Good find! Needs a style fix

@edenhill edenhill added this to the v1.5.0 milestone Jul 17, 2020
@edenhill edenhill added the bug label Jul 17, 2020
If there are control messages in the topic, rd_kafka_destroy will
hang if only rd_kafka_consume_batch is used, rd_kafka_consume and
rd_kafka_consume_callback don't have this issue.

Release these messages before returning to the application.
@pf-qiu pf-qiu force-pushed the destroy_control_message branch from a838e31 to 1a797f4 Compare July 19, 2020 05:17
@edenhill edenhill merged commit c157bd3 into confluentinc:master Jul 20, 2020
@edenhill
Copy link
Contributor

Thanks for this fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants