-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[improve][broker] Add http produce backlog quota check #25189
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
base: master
Are you sure you want to change the base?
[improve][broker] Add http produce backlog quota check #25189
Conversation
- Add http produce backlog quota check for both destination_storage and message_age Signed-off-by: Dream95 <zhou_8621@163.com>
Denovo1998
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it does a backlog quota check for each message, maybe it can be optimized?
- In internalPublishMessages(...), first calculate which partition(s) the current request will write to (actually, you have already calculated in round-robin which partition each message goes to).
- Perform once for each partition:
- getTopic(partitionTopic)
- checkBacklogQuotaExceeded(...destination_storage + message_age)
- After all checks pass, then enter the actual publish loop
- If the check for a certain partition fails: mark all the message results corresponding to that partition as failed (no longer attempt to publish), other partitions can continue.
| topicObj.checkBacklogQuotaExceeded(defaultProducerName, | ||
| BacklogQuota.BacklogQuotaType.destination_storage), | ||
| topicObj.checkBacklogQuotaExceeded(defaultProducerName, | ||
| BacklogQuota.BacklogQuotaType.message_age)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we pass in message.getProducerName(); here?
| if (log.isDebugEnabled()) { | ||
| log.debug("Fail to publish single messages to topic {}: {} ", | ||
| topicName, e.getCause()); | ||
| log.debug("Fail to publish single message to topic {}: {}", topic, ex.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can first use FutureUtil.unwrapCompletionException.
| .topic(topicName) | ||
| .create(); | ||
| producer.send("backlog-message"); | ||
| Thread.sleep(2000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it can be replaced with a "poll until quota exceeded (with timeout)" method.
Motivation
The binary protocol already performs check backlog quota in
ServerCnxwhen creating producers, but the HTTP REST API path was missing this validation.Modifications
publishSingleMessageToPartitionmethod for bothdestination_storageandmessage_agequota typesThe implementation follows the same pattern used in
ServerCnx.handleProducerwhere backlog quota checks are performed before allowing message production.Verifying this change
This change is already covered by existing tests, such as:
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Changes to REST endpoints:
/persistent/{tenant}/{namespace}/{topic}/partitions/{partition}) now properly enforces backlog quota limits before publishing messages. When backlog quota is exceeded, the request will fail with an appropriate error, consistent with the binary protocol behavior.Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: Dream95#4