MINOR: Use DLQ topic for share group id to enable DLQ.#22436
Conversation
Co-authored-by: Claude noreply@anthropic.com
smjn
requested review from
AndrewJSchofield and
apoorvmittal10
and removed request for
apoorvmittal10
github-actions
Bot
added
core
apoorvmittal10
left a comment
apoorvmittal10
left a comment
There was a problem hiding this comment.
Thanks for the PR, a basic question I have. Left as a comment.
| public String errorsDLQTopicName(String groupId) { | ||
| return manager.groupConfig(groupId) | ||
| .map(GroupConfig::errorsDLQTopicName) | ||
| .orElse(GroupConfig.ERRORS_DEADLETTERQUEUE_TOPIC_NAME_DEFAULT); // In case no group config |
There was a problem hiding this comment.
The KIP says following: The broker does not by default automatically create DLQ topics, because that again would give a convoluted way of getting the broker to create a topic with a particular name. hence why do we want to default to a topic name?
There was a problem hiding this comment.
@apoorvmittal10 Thanks for the review - the default is just a SENTINEL - its an empty String "". We have the check in SharePartition which looking at empty name does not invoke ARCHIVING flow.
DLQ topic auto creation is controlled via errors.deadletterqueue.auto.create.topics.enable. But both in case of this config being enabled for disabled - user needs to provide a DLQ topic name for a share group on which they want to invoke the DLQ flow. We do not create the topic, if config is set, unless it is specified on the group.
There was a problem hiding this comment.
But when there is never default to be supported then why do you need to return the default value from this method?
There was a problem hiding this comment.
Ok, will make it optional
apoorvmittal10
left a comment
apoorvmittal10
left a comment
There was a problem hiding this comment.
LGTM, one suggestion.
| * is present, then the value from the group config is used. Otherwise, empty optional is returned. | ||
| * | ||
| * @param groupId The group id for which the DLQ topic name is to be fetched. | ||
| * @return DLQ topic name for the share group. |
There was a problem hiding this comment.
Can you please update this as now optional is being returned.
2 participants
config
share.version=2to enable DLQ flow. However, this in notcorrect.
share group is ESSENTIAL for DLQ flows to start.
SharePartition where the configProvider is leveraged to fetch the DLQ
topic config applied to the share group. In case the topic name is
empty, DLQ flow does not execute.
with mock DLQ manager to assert on the enqueue calls.
Reviewers: Apoorv Mittal apoorvmittal10@gmail.com