fix: Timeout settings in SqsTemplate #1250
Conversation
* Timeouts were being set by calling Duration#getSecondsPart whic is alwawys a number between 0 and 59 * Update to use Duration#getSeconds to get the full timeout specified, in seconds
joey
requested review from
maciejwalkowiak,
MatejNedic and
tomazfernandes
as code owners
There was a problem hiding this comment.
LGTM but @tomazfernandes take a look in case there are any side effects i am not aware of
|
Looks good! It might be worth it checking if ContainerOptions has the same problem though. |
I didn't fine any other uses of the |
| @@ -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) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We can go with 2 approaches.
-
Yours by rounding to maximum integer there is, which is totally valid.
-
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!
📢 Type of change
📜 Description
💡 Motivation and Context
This supports using a visibility time and polling timeout that's greater than 60 seconds.
💚 How did you test it?
Updated one of the unit tests for SqsTemplate
📝 Checklist
🔮 Next steps