Skip to content
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

Apply backpressure in misc places to prevent resource exhaustion issues #177

Conversation

hiddenalpha
Copy link
Member

@hiddenalpha hiddenalpha commented May 16, 2024

Relates to SDCISA-15833, Probably fixes #170 .

Usage example (replace dots with your respective quota):

  RedisQues.builder()
      .withRedisMonitoringReqQuota(...)
      .withCheckQueueRequestsQuota(...)
      .withQueueStatsRequestQuota(...)
      .withGetQueuesItemsCountRedisRequestQuota(...)
      .build();

If nothing is configured, RedisQues will use INT_MAX to emulate the behavior as it was before.

return;
}
if (redisRequestQuota.availablePermits() <= 0) {
event.reply(new NoStackReplyException(ReplyFailure.RECIPIENT_FAILURE, 507, "Server too busy. Rejecting this GetQueuesItemsCount request"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious to know: Do the clients using redisques typically expect such feedback. I mean can they handle it fine and continue processing / recover fine.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: I think we end up in the error case below in the Gateleen MonitoringHandler. It is currently considered en "error", we would need to relax it to a "warn", hmm, maybe OK, do you plan to implement some slowdown around here?

public void updateLastUsedQueueSizeInformation(final String queue) {
    log.trace("About to update last used Queue size counter");
    vertx.eventBus().request(getRedisquesAddress(), buildGetQueueItemsCountOperation(queue), (Handler<AsyncResult<Message<JsonObject>>>) reply -> {
        if (reply.succeeded() && OK.equals(reply.result().body().getString(STATUS))) {
            final long count = reply.result().body().getLong(VALUE);
            vertx.eventBus().publish(getMonitoringAddress(), new JsonObject().put(METRIC_NAME, prefix + LAST_USED_QUEUE_SIZE_METRIC).put(METRIC_ACTION, "update").put("n", count));
        } else {
            log.error("Error gathering queue size for queue '{}'", queue);
        }
    });
}

Copy link
Member Author

@hiddenalpha hiddenalpha May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, those are not the same operations. Seems as there was some trouble naming things. As those operation names sound confusingly similar. I don't understand how they actually differ.

Up to now the only downstream I found was QueueStatsService.

Can you confirm/disconfirm?

hiddenalpha added a commit to hiddenalpha/vertx-redisques that referenced this pull request May 21, 2024
hiddenalpha added a commit to hiddenalpha/vertx-redisques that referenced this pull request May 21, 2024
hiddenalpha added a commit to hiddenalpha/gateleen that referenced this pull request May 21, 2024
hiddenalpha added a commit to hiddenalpha/vertx-redisques that referenced this pull request May 21, 2024
@@ -16,6 +16,10 @@ class WastefulExceptionFactory implements ExceptionFactory {
WastefulExceptionFactory() {
}

public Exception newException(String message, Throwable cause) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tried to understand how it all works, but in this context I would except that we get some HeavyweightException (or maybe StacktraceException) compared to the ThriftyExceptionFactory that would then give as some LightweightException (StacktraceLessException). It would make things easier to understand I believe. But is this then not convenient to work with?

hiddenalpha added a commit to hiddenalpha/vertx-rest-storage that referenced this pull request May 23, 2024
hiddenalpha added a commit to hiddenalpha/vertx-rest-storage that referenced this pull request May 23, 2024
@hiddenalpha hiddenalpha merged commit b00ebff into swisspost:develop May 24, 2024
1 check passed
hiddenalpha added a commit to hiddenalpha/vertx-redisques that referenced this pull request May 24, 2024
OMG! What a conflict mess.

Conflicts:
      src/main/java/org/swisspush/redisques/QueueStatsService.java
      src/main/java/org/swisspush/redisques/RedisQues.java
      src/main/java/org/swisspush/redisques/action/GetQueuesItemsCountAction.java
      src/main/java/org/swisspush/redisques/handler/GetQueuesItemsCountHandler.java
      src/main/java/org/swisspush/redisques/handler/RedisquesHttpRequestHandler.java
      src/main/java/org/swisspush/redisques/util/QueueActionFactory.java
      src/main/java/org/swisspush/redisques/util/QueueStatisticsCollector.java

Related: SDCISA-15833, swisspost#170, swisspost#177, swisspost/vertx-rest-storage#186, swisspost/gateleen#577
hiddenalpha added a commit to hiddenalpha/vertx-redisques that referenced this pull request May 24, 2024
hiddenalpha added a commit to hiddenalpha/gateleen that referenced this pull request May 24, 2024
hiddenalpha added a commit to hiddenalpha/vertx-redisques that referenced this pull request May 24, 2024
hiddenalpha added a commit to hiddenalpha/gateleen that referenced this pull request Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

checkQueues can lead to redis waiting queue full error
3 participants