[receiver/kafkareceiver] Add support for disabling KIP-320 (truncation detection via leader epoch) for Franz-Go#42796
Conversation
…n detection via leader epoch) for Franz-Go Signed-off-by: Paulo Dias <paulodias.gm@gmail.com>
paulojmdias
requested review from
a team,
MovieStoreGuy and
axw
as code owners
Signed-off-by: Paulo Dias <paulodias.gm@gmail.com>
axw
left a comment
axw
left a comment
There was a problem hiding this comment.
Thanks @paulojmdias! Code looks great.
| - `group_id` (default = otel-collector): The consumer group that receiver will be consuming messages from | ||
| - `client_id` (default = otel-collector): The consumer client ID that receiver will use | ||
| - `rack_id` (default = ""): The rack identifier for this client. When set and brokers are configured with a rack-aware replica selector, the client will prefer fetching from the closest replica. | ||
| - `disable_leader_epoch` (default = false): When enabled, the consumer clears leader epoch information from fetch offsets. This disables `KIP-320` truncation detection in franz-go, which avoids compatibility issues with brokers that don’t fully support the feature (e.g., Azure Event Hubs), at the cost of losing automatic log-truncation safety. |
There was a problem hiding this comment.
Could we make this "enable_leader_epoch" or "use_leader_epoch" to avoid the double negative?
Also, what do you think about labeling this config as experimental? If the issue in Event Hubs is fixed (#42226 (comment)) then maybe this config can go away.
There was a problem hiding this comment.
@axw I renamed it to use_leader_epoch, as I think it makes more sense. I also marked this as experimental in the README.md file. Do we need to mark it somewhere else?
There was a problem hiding this comment.
Sounds good, thanks. I think the README is good enough. @MovieStoreGuy please let us know if there's something else we should do to mark the feature as experimental.
Signed-off-by: Paulo Dias <paulodias.gm@gmail.com>
| // standard "client.rack" setting. By default, this is empty. | ||
| RackID string `mapstructure:"rack_id"` | ||
|
|
||
| // UseLeaderEpoch forces the collector to clear the leader epoch from |
There was a problem hiding this comment.
Sorry if I was unclear, when I said to invert the name I also meant to invert the meaning. So "use_leader_epoch: true" is what franz-go would do be default -- it would use the leader epoch returned by the brokers. Setting to false would disable KIP-320. Does this sound sensible?
There was a problem hiding this comment.
Makes sense @axw. I updated the logic and also the docs. I also updated the tests for the kafkatopicsobserver to ensure will match the default config.
Please review to see if it makes sense to you now.
There was a problem hiding this comment.
The code looks good, thanks. This doc comment needs to be updated now though
There was a problem hiding this comment.
I updated the comment to match the README.md 👍
Co-authored-by: Andrew Wilkins <axwalk@gmail.com>
Signed-off-by: Paulo Dias <paulodias.gm@gmail.com>
Signed-off-by: Paulo Dias <paulodias.gm@gmail.com>
Signed-off-by: Paulo Dias <paulodias.gm@gmail.com>
axw
left a comment
axw
left a comment
There was a problem hiding this comment.
Thanks @paulojmdias LGTM, just needs a doc comment update
| // standard "client.rack" setting. By default, this is empty. | ||
| RackID string `mapstructure:"rack_id"` | ||
|
|
||
| // UseLeaderEpoch forces the collector to clear the leader epoch from |
There was a problem hiding this comment.
The code looks good, thanks. This doc comment needs to be updated now though
Signed-off-by: Paulo Dias <paulodias.gm@gmail.com>
axw
added
the
ready to merge
#### Description I would like to propose adding @paulojmdias as a code owner of the Kafka components. He has been doing excellent work across all of those components (and more besides!). - #42507 - #42796 - #43105 - #42181 - #42327 - #42507 - #42796 - #43019 - #43083 - #43105 #### Link to tracking issue N/A #### Testing N/A #### Documentation N/A
#### Description I would like to propose adding @paulojmdias as a code owner of the Kafka components. He has been doing excellent work across all of those components (and more besides!). - open-telemetry#42507 - open-telemetry#42796 - open-telemetry#43105 - open-telemetry#42181 - open-telemetry#42327 - open-telemetry#42507 - open-telemetry#42796 - open-telemetry#43019 - open-telemetry#43083 - open-telemetry#43105 #### Link to tracking issue N/A #### Testing N/A #### Documentation N/A
4 participants
Description
Add a new
disable_leader_epochoption to the Kafka receiver’s client config. When set, the franz-go consumer clears leader epoch information from fetch offsets, effectively disabling KIP-320 truncation detection. This provides compatibility with brokers that don’t support leader epochs (e.g., Azure Event Hub w/ Premium SKU), while leaving the default behavior unchanged.Link to tracking issue
Fixes #42226
Testing
Added some tests to validate this implementation.
Missing tests in Azure Event Hub with Premium SKU to ensure this is mitigated
Documentation
Updated README.md with the new option.