[KIP-714] Fix idle ratio calculation for non forwarded queues #5017
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the idle ratio calculation for partition queues that are not forwarded to the main poll queue by switching from per-instance to per-queue timestamps and adjusting related polling logic. Key changes include:
- Adding new per-queue timestamp fields (rkq_ts_last_poll_start and rkq_ts_last_poll_end) in the queue structure.
- Refactoring queue serve/consume functions to pass an additional flag and use the new per-queue timestamps.
- Updating inline functions and call sites in rdkafka_int.h and rdkafka.c to use the per-queue approach.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/rdkafka_queue.h | Introduces new per-queue timestamp fields and prototypes for consume operations. |
| src/rdkafka_queue.c | Updates initialization and serve functions to use per-queue timestamps and propagate a consume flag. |
| src/rdkafka_int.h | Modifies inline poll functions to operate on per-queue timestamps instead of global instance fields. |
| src/rdkafka.c | Adjusts poll and consume call sites to pass the queue pointer to the updated functions. |
Comments suppressed due to low confidence (2)
src/rdkafka_queue.c:390
- [nitpick] Consider renaming the parameter 'consume_call' to 'is_consume_call' to more clearly indicate that it flags a consume operation.
static rd_kafka_op_t *rd_kafka_q_pop_serve0(rd_kafka_q_t *rkq, ... , rd_bool_t consume_call) {
src/rdkafka_int.h:1168
- [nitpick] Since this function now operates on per-queue timestamps, consider renaming it (e.g., to 'rd_kafka_queue_poll_start') to better reflect its behavior.
static RD_INLINE RD_UNUSED void rd_kafka_app_poll_start(rd_kafka_t *rk, rd_kafka_q_t *rkq, rd_ts_t now, rd_bool_t is_blocking) {
8f5aafc to
a7ff5bb
Compare
d2d3692 to
99a1941
Compare
anchitj
reviewed
Jun 9, 2025
anchitj
approved these changes
Jun 9, 2025
942fff1 to
9eee36a
Compare
anchitj
approved these changes
Jun 11, 2025
…oll_start" in test 0056. Partition queues can be not forwarded to the main poll queue so these timestamps must be per-queue instead of per-instance. We avoid checking if app polled should be called in case of internal poll calls where we're sure it's not a consume call, we also avoid checking it if "app polled" was already called by a dedicated consume function.
9eee36a to
46e5582
Compare
anchitj
approved these changes
Jun 16, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix for devel assert "rkq->rkq_ts_last_poll_end >= rkq->rkq_ts_last_poll_start" in test 0056.
Partition queues can be not forwarded to the main poll queue so these timestamps must be per-queue instead of per-instance. We avoid checking if app polled should be called in case of internal poll calls where we're sure it's not a consume call, we also avoid checking it if "app polled" was already called by a dedicated consume function.