Fix timeout handling in QueueMessageChannel.java

spring-attic / spring-cloud-aws

All development has moved to https://github.com/awspring/spring-cloud-aws Integration for Amazon Web Services APIs with Spring

https://awspring.io/

Apache License 2.0

590 stars 373 forks source link

Fix timeout handling in QueueMessageChannel.java #769

Closed jorgenota closed 3 years ago

jorgenota commented 3 years ago

:loudspeaker: Type of change

[X] Bugfix
[ ] New feature
[ ] Enhancement
[ ] Refactoring

:scroll: Description

Minor changes to better handle timeout parameter when receiving messages from an SQS queue.

:bulb: Motivation and Context

I suggest this fixes to align with PollableChannel interface:

According to javadoc of PollableChannel, MessageChannel.INDEFINITE_TIMEOUT should be used to represent indefinite timeout (instead of 0).
The timeout parameter of PollableChannel.receive() method represents the timeout in milliseconds (so it should be transformed into seconds when building the ReceiveMessageRequest).

:pencil: Checklist

[X ] I reviewed submitted code
[ ] I added tests to verify changes
[ ] I updated reference documentation to reflect the change
[ ] All tests passing
[ ] No breaking changes

:crystal_ball: Next steps

pivotal-issuemaster commented 3 years ago

@jorgenota Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster commented 3 years ago

@jorgenota Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster commented 3 years ago

@jorgenota Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster commented 3 years ago

@jorgenota Thank you for signing the Contributor License Agreement!

maciejwalkowiak commented 3 years ago

0 in this case means short polling - not indefinite timeout. With milliseconds - agreed this should be changed, but we need to think if we do it for 2.4 or 3.0.

Can you move this PR to https://github.com/awspring/spring-cloud-aws? This repository is only used for the 2.2.x maintenance.

jorgenota commented 3 years ago

0 in this case means short polling - not indefinite timeout.

Yes, '0' means short polling. According to javadoc of PollableChannel, receive() method "Receive a message from this channel, blocking indefinitely if necessary". That's why I suggested using MessageChannel.INDEFINITE_TIMEOUT constant in line 204 (instead of 0).

Am I right? Should I move my PR as it is right now (or change only the transformation to seconds)?

Can you move this PR to https://github.com/awspring/spring-cloud-aws? This repository is only used for the 2.2.x maintenance.

Of course, thanks for the clarification.

maciejwalkowiak commented 3 years ago

Replaced with https://github.com/awspring/spring-cloud-aws/pull/123