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.

Sign up for GitHub

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

Jump to bottom

fix: Timeout settings in SqsTemplate #1250

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: Timeout settings in SqsTemplate #1250

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4bd8bf7
fix: Timeout settings in SqsTemplate
joey Oct 2, 2024
3ec6b0a
Convert the long seconds into an int
joey Oct 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions spring-cloud-aws-sqs/src/main/java/io/awspring/cloud/sqs/operations/SqsTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,11 +602,11 @@ private ReceiveMessageRequest doCreateReceiveMessageRequest(Duration pollTimeout
ReceiveMessageRequest.Builder builder = ReceiveMessageRequest.builder().queueUrl(attributes.getQueueUrl())
.maxNumberOfMessages(maxNumberOfMessages).messageAttributeNames(this.messageAttributeNames)
.attributeNamesWithStrings(this.messageSystemAttributeNames)
.waitTimeSeconds(pollTimeout.toSecondsPart());
.waitTimeSeconds(toInt(pollTimeout.toSeconds()));
if (additionalHeaders.containsKey(SqsHeaders.SQS_VISIBILITY_TIMEOUT_HEADER)) {
builder.visibilityTimeout(
getValueAs(additionalHeaders, SqsHeaders.SQS_VISIBILITY_TIMEOUT_HEADER, Duration.class)
.toSecondsPart());
toInt(getValueAs(additionalHeaders, SqsHeaders.SQS_VISIBILITY_TIMEOUT_HEADER, Duration.class)
.toSeconds()));
}
if (additionalHeaders.containsKey(SqsHeaders.SQS_RECEIVE_REQUEST_ATTEMPT_ID_HEADER)) {
builder.receiveRequestAttemptId(
Expand All @@ -616,6 +616,15 @@ private ReceiveMessageRequest doCreateReceiveMessageRequest(Duration pollTimeout
return builder.build();
}

// Convert a long value to an int. Values larger than Integer.MAX_VALUE are set to Integer.MAX_VALUE
private int toInt(long longValue) {
Copy link
Author

Choose a reason for hiding this comment

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

I didn't want to use Math.toIntExact() as it will throw if the long value is out of the range and I thought pinning to the max integer would be better behavior. If we think it's unlikely enough that someone would set a timeout that high (over 63 years), then I'm happy to switch to the built in.

I didn't bother pinning too small of negative values as I don't think anyone would intentionally set a negative timeout.

Copy link
Member

Choose a reason for hiding this comment

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

We can go with 2 approaches.

  1. Yours by rounding to maximum integer there is, which is totally valid.

  2. Or follow AWS SDK approach where we would (validate) throw exception with stating that value is above limit.

I personally would maybe want to go with approach number 2 just to be aligned, but both approaches are totally valid.
But tbh we are now nitpicking since no one will probably want or will set timeout that big. So for me this is fine!

if (longValue > Integer.MAX_VALUE) {
return Integer.MAX_VALUE;
}

return (int) longValue;
}

private <V> V getValueAs(Map<String, Object> headers, String headerName, Class<V> valueClass) {
return valueClass.cast(headers.get(headerName));
}
Expand Down
6 changes: 3 additions & 3 deletions spring-cloud-aws-sqs/src/test/java/io/awspring/cloud/sqs/operations/SqsTemplateTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -937,7 +937,7 @@ void shouldReceiveFromOptions() {
.willReturn(CompletableFuture.completedFuture(deleteResponse));
SqsOperations template = SqsTemplate.newSyncTemplate(mockClient);
Optional<Message<String>> receivedMessage = template.receive(from -> from.queue(queue)
.pollTimeout(Duration.ofSeconds(1)).visibilityTimeout(Duration.ofSeconds(5))
.pollTimeout(Duration.ofSeconds(61)).visibilityTimeout(Duration.ofSeconds(65))
.additionalHeader(headerName1, headerValue1).additionalHeaders(Map.of(headerName2, headerValue2)),
String.class);
assertThat(receivedMessage).isPresent().hasValueSatisfying(message -> {
Expand All @@ -949,8 +949,8 @@ void shouldReceiveFromOptions() {
then(mockClient).should().receiveMessage(captor.capture());
ReceiveMessageRequest request = captor.getValue();
assertThat(request.maxNumberOfMessages()).isEqualTo(1);
assertThat(request.visibilityTimeout()).isEqualTo(5);
assertThat(request.waitTimeSeconds()).isEqualTo(1);
assertThat(request.visibilityTimeout()).isEqualTo(65);
assertThat(request.waitTimeSeconds()).isEqualTo(61);
}

@Test
Expand Down