KAFKA-20410: Add DLQ configuration parameters for Share Groups (KIP-1191)#21979
KAFKA-20410: Add DLQ configuration parameters for Share Groups (KIP-1191)#21979sjhajharia wants to merge 6 commits intoapache:trunkfrom
Conversation
| /** Dead Letter Queue Configurations (KIP-1191) **/ | ||
|
|
||
| // Cluster-level DLQ configs | ||
| public static final String ERRORS_DEADLETTERQUEUE_AUTO_CREATE_TOPICS_ENABLE_CONFIG = "errors.deadletterqueue.auto.create.topics.enable"; |
There was a problem hiding this comment.
All configs starts with the same prefix. Why don’t we follow the same pattern for those?
There was a problem hiding this comment.
We are intentionally not including the group.share. prefix on these configs to keep them generic enough so that in the future, we can re-use them as and when needed and maintain compatibility still.
AndrewJSchofield
left a comment
There was a problem hiding this comment.
Thanks for the PR. I've been a bit picky about the doc strings for the configs because they will go into the documentation.
| "DLQ topic name must not start with '__'"); | ||
| } | ||
|
|
||
| // If auto-create is enabled and the topic name is specified, it must start with the configured prefix |
There was a problem hiding this comment.
I don't think this validation is quite what I intended. It does the right thing in the wrong way for a subset of the situations the feature needs to cope with.
I don't think the validation of the DLQ topic name config depends upon the auto-create config at all. If the DLQ topic name for the group does not satisfy the broker-level topic name prefix, that is a failure at the time we need to enqueue a record.
There was a problem hiding this comment.
Thanks for the comment. I have removed that validation from here.
|
|
||
| // Cluster-level DLQ configs | ||
| public static final String ERRORS_DEADLETTERQUEUE_AUTO_CREATE_TOPICS_ENABLE_CONFIG = "errors.deadletterqueue.auto.create.topics.enable"; | ||
| public static final boolean ERRORS_DEADLETTERQUEUE_AUTO_CREATE_TOPICS_ENABLE_DEFAULT = false; |
There was a problem hiding this comment.
Given that these configs are intended to be more general than share groups because we might introduce a way to handle poisoned messages for other group types in the future, I would push these configs up a level and not define them in ShareGroupConfig.
There was a problem hiding this comment.
Thanks for the feedback. Do you think the GroupCoordinatorConfig class may be a better seat for these configs? I was thinking of the GroupConfig class too, but those config seem very group specific, while we are talking about both cluster level and Group level configs.
The other alternative I could think was to include errors.deadletterqueue.auto.create.topics.enable and errors.deadletterqueue.topic.name.prefix in the GroupCoordinatorConfig class whole the other two can sit in GroupConfig class.
Please let me know which is the direction which seems better here, or if there is another alternative. TIA!
There was a problem hiding this comment.
@sjhajharia Sorry for the delay. I think that GroupConfig is the best.
There was a problem hiding this comment.
Thanks for the suggestion. I have updated the PR accordingly.
400dc85 to
837f334
Compare
Summary
This PR adds the configuration foundation for Share Groups Dead-Letter
Queue (DLQ) functionality as specified in KIP-1191: Dead-letter queues
for share
groups.
Changes
New Configurations
Cluster-level configs (GroupConfig.java):
errors.deadletterqueue.auto.create.topics.enable(default:false)errors.deadletterqueue.topic.name.prefix(default:"dlq.")Group-level configs (GroupConfig.java):
errors.deadletterqueue.topic.name(default:"")errors.deadletterqueue.copy.record.enable(default:false)true: Copy full original record to DLQfalse: Copy only context metadata (offset, delivery count,reason)
Topic-level config (TopicConfig.java):
errors.deadletterqueue.group.enable(default: not set)Validation Logic
__(reserved for internaltopics)
Tests
Added 7 comprehensive test cases in
ShareGroupConfigTest:testDLQConfigDefaults()- Verify default valuestestDLQConfigCustomValues()- Validate custom configurationstestDLQTopicNameCannotStartWithDoubleUnderscore()- Reject__prefix
testDLQTopicNameMustMatchPrefixWhenAutoCreateEnabled()- Enforceprefix with auto-create
Reviewers: David Jacot david.jacot@gmail.com, Andrew Schofield
aschofield@confluent.io