eolivelli commented on a change in pull request #12874:
URL: https://github.com/apache/pulsar/pull/12874#discussion_r755527273
##########
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -1189,65 +1190,70 @@ protected void handleProducer(final CommandProducer
cmdProducer) {
service.getOrCreateTopic(topicName.toString()).thenAccept((Topic
topic) -> {
// Before creating producer, check if backlog quota exceeded
// on topic for size based limit and time based limit
- for (BacklogQuota.BacklogQuotaType backlogQuotaType :
BacklogQuota.BacklogQuotaType.values()) {
- if (topic.isBacklogQuotaExceeded(producerName,
backlogQuotaType)) {
- IllegalStateException illegalStateException = new
IllegalStateException(
- "Cannot create producer on topic with backlog
quota exceeded");
- BacklogQuota.RetentionPolicy retentionPolicy = topic
- .getBacklogQuota(backlogQuotaType).getPolicy();
- if (retentionPolicy ==
BacklogQuota.RetentionPolicy.producer_request_hold) {
- commandSender.sendErrorResponse(requestId,
-
ServerError.ProducerBlockedQuotaExceededError,
- illegalStateException.getMessage());
- } else if (retentionPolicy ==
BacklogQuota.RetentionPolicy.producer_exception) {
- commandSender.sendErrorResponse(requestId,
-
ServerError.ProducerBlockedQuotaExceededException,
- illegalStateException.getMessage());
- }
-
producerFuture.completeExceptionally(illegalStateException);
- producers.remove(producerId, producerFuture);
- return;
+ CompletableFuture<Void> backlogQuotaCheckFuture =
CompletableFuture.allOf(
+ topic.checkBacklogQuotaExceeded(producerName,
BacklogQuotaType.destination_storage),
+ topic.checkBacklogQuotaExceeded(producerName,
BacklogQuotaType.message_age));
+ backlogQuotaCheckFuture.exceptionally(throwable -> {
+ //throwable should be CompletionException holding
TopicBacklogQuotaExceededException
+ BrokerServiceException.TopicBacklogQuotaExceededException
exception =
+
(BrokerServiceException.TopicBacklogQuotaExceededException)
throwable.getCause();
+ IllegalStateException illegalStateException = new
IllegalStateException(exception);
+ BacklogQuota.RetentionPolicy retentionPolicy =
exception.getRetentionPolicy();
+ if (retentionPolicy ==
BacklogQuota.RetentionPolicy.producer_request_hold) {
+ commandSender.sendErrorResponse(requestId,
+ ServerError.ProducerBlockedQuotaExceededError,
+ illegalStateException.getMessage());
+ } else if (retentionPolicy ==
BacklogQuota.RetentionPolicy.producer_exception) {
+ commandSender.sendErrorResponse(requestId,
+
ServerError.ProducerBlockedQuotaExceededException,
+ illegalStateException.getMessage());
}
- }
- // Check whether the producer will publish encrypted messages
or not
- if ((topic.isEncryptionRequired() ||
encryptionRequireOnProducer) && !isEncrypted) {
- String msg = String.format("Encryption is required in %s",
topicName);
- log.warn("[{}] {}", remoteAddress, msg);
- commandSender.sendErrorResponse(requestId,
ServerError.MetadataError, msg);
+
producerFuture.completeExceptionally(illegalStateException);
producers.remove(producerId, producerFuture);
- return;
- }
+ return null;
+ });
- disableTcpNoDelayIfNeeded(topicName.toString(), producerName);
+ backlogQuotaCheckFuture.thenRun(() -> {
Review comment:
Which thread will execute this code?
The current thread (in case we reach here and the futures are already
completed) or some other thread on completion of the one of the two futures
above.
I am not sure we have control over what's happening here
##########
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
##########
@@ -2518,17 +2528,26 @@ public boolean isSizeBacklogExceeded() {
* @return determine if backlog quota enforcement needs to be done for
topic based on time limit
*/
public boolean isTimeBacklogExceeded() {
+ try {
+ return checkTimeBacklogExceeded().get();
+ } catch (Throwable e) {
Review comment:
This is a code smell
##########
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -1189,65 +1190,70 @@ protected void handleProducer(final CommandProducer
cmdProducer) {
service.getOrCreateTopic(topicName.toString()).thenAccept((Topic
topic) -> {
// Before creating producer, check if backlog quota exceeded
// on topic for size based limit and time based limit
- for (BacklogQuota.BacklogQuotaType backlogQuotaType :
BacklogQuota.BacklogQuotaType.values()) {
- if (topic.isBacklogQuotaExceeded(producerName,
backlogQuotaType)) {
- IllegalStateException illegalStateException = new
IllegalStateException(
- "Cannot create producer on topic with backlog
quota exceeded");
- BacklogQuota.RetentionPolicy retentionPolicy = topic
- .getBacklogQuota(backlogQuotaType).getPolicy();
- if (retentionPolicy ==
BacklogQuota.RetentionPolicy.producer_request_hold) {
- commandSender.sendErrorResponse(requestId,
-
ServerError.ProducerBlockedQuotaExceededError,
- illegalStateException.getMessage());
- } else if (retentionPolicy ==
BacklogQuota.RetentionPolicy.producer_exception) {
- commandSender.sendErrorResponse(requestId,
-
ServerError.ProducerBlockedQuotaExceededException,
- illegalStateException.getMessage());
- }
-
producerFuture.completeExceptionally(illegalStateException);
- producers.remove(producerId, producerFuture);
- return;
+ CompletableFuture<Void> backlogQuotaCheckFuture =
CompletableFuture.allOf(
+ topic.checkBacklogQuotaExceeded(producerName,
BacklogQuotaType.destination_storage),
+ topic.checkBacklogQuotaExceeded(producerName,
BacklogQuotaType.message_age));
+ backlogQuotaCheckFuture.exceptionally(throwable -> {
+ //throwable should be CompletionException holding
TopicBacklogQuotaExceededException
+ BrokerServiceException.TopicBacklogQuotaExceededException
exception =
Review comment:
What happens if there is something else?
A ClassCastException or a NPE.
This will break the system
I believe we should handle the case in which this expectation is not met
##########
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
##########
@@ -1189,65 +1190,70 @@ protected void handleProducer(final CommandProducer
cmdProducer) {
service.getOrCreateTopic(topicName.toString()).thenAccept((Topic
topic) -> {
// Before creating producer, check if backlog quota exceeded
// on topic for size based limit and time based limit
- for (BacklogQuota.BacklogQuotaType backlogQuotaType :
BacklogQuota.BacklogQuotaType.values()) {
- if (topic.isBacklogQuotaExceeded(producerName,
backlogQuotaType)) {
- IllegalStateException illegalStateException = new
IllegalStateException(
- "Cannot create producer on topic with backlog
quota exceeded");
- BacklogQuota.RetentionPolicy retentionPolicy = topic
- .getBacklogQuota(backlogQuotaType).getPolicy();
- if (retentionPolicy ==
BacklogQuota.RetentionPolicy.producer_request_hold) {
- commandSender.sendErrorResponse(requestId,
-
ServerError.ProducerBlockedQuotaExceededError,
- illegalStateException.getMessage());
- } else if (retentionPolicy ==
BacklogQuota.RetentionPolicy.producer_exception) {
- commandSender.sendErrorResponse(requestId,
-
ServerError.ProducerBlockedQuotaExceededException,
- illegalStateException.getMessage());
- }
-
producerFuture.completeExceptionally(illegalStateException);
- producers.remove(producerId, producerFuture);
- return;
+ CompletableFuture<Void> backlogQuotaCheckFuture =
CompletableFuture.allOf(
+ topic.checkBacklogQuotaExceeded(producerName,
BacklogQuotaType.destination_storage),
+ topic.checkBacklogQuotaExceeded(producerName,
BacklogQuotaType.message_age));
+ backlogQuotaCheckFuture.exceptionally(throwable -> {
Review comment:
Which thread will execute this code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
