Replace TimeUnit with Duration in Redis queue operations#10836
Conversation
artembilan
left a comment
artembilan
left a comment
There was a problem hiding this comment.
Please, add your name to the @author list of all the affected classes.
Thank you!
| @@ -191,7 +191,7 @@ private void handlePopException(Exception e) { | |||
| private void receiveAndReply() { | |||
| byte[] value; | |||
| try { | |||
| value = this.boundListOperations.rightPop(this.receiveTimeout, TimeUnit.MILLISECONDS); | |||
| value = this.boundListOperations.rightPop(Duration.ofMillis(this.receiveTimeout)); | |||
There was a problem hiding this comment.
As we discussed, let's have an additional setReceiveTimeout(Duration)!
| @@ -241,10 +241,10 @@ private void popMessageAndSend() { | |||
| byte[] value = null; | |||
| try { | |||
| if (this.rightPop) { | |||
| value = this.boundListOperations.rightPop(this.receiveTimeout, TimeUnit.MILLISECONDS); | |||
| value = this.boundListOperations.rightPop(Duration.ofMillis(this.receiveTimeout)); | |||
There was a problem hiding this comment.
Any clues why this class was not fixed same way as others?
artembilan
left a comment
artembilan
left a comment
There was a problem hiding this comment.
Please, rebase to the latest main.
We got some fixes according to the breaking changes from upstream projects.
Thanks
cppwfs
force-pushed
the
SI-redis-duration
branch
from
57c6c01 to
6c00e1b
Compare
| * ISO-8601 duration format. | ||
| * @since 7.1 | ||
| */ | ||
| public void setReceiveTimeout(String receiveTimeout) { |
There was a problem hiding this comment.
This is nice, but not what should to be done in this component.
| * | ||
| * @since 7.1 | ||
| */ | ||
| public abstract class DurationUtil { |
There was a problem hiding this comment.
I think we don't need this utility.
Looks like there is one in SF already: DurationFormatterUtils.
I still don't see a PropertyEditor for property placeholders, but that's different story.
I guess we can fix that PeriodicTriggerFactoryBean, respectively, but we definitely don't need this utility in our project.
And I also believe that we should not provide string-based setters: let end-user to do the parsing before calling our component!
There was a problem hiding this comment.
The issue that I ran into is that the XML Bean definitions pass the receiveTimeout as a String and they can't be converted to a Duration. For some reason it would not use the long overload. Any suggestions?
There was a problem hiding this comment.
Yeah... That's exactly why we need a ProperyEditor for Duration.
We can raise an issue for SF.
There is also another problem for XML bean definitions: if we have overloaded setters, there is no guarantee which one would be tried by JavaBeans introspection.
The solution is to not have overloaded setters, but name a new one as setReceiveDuration(Duration).
Makes sense?
There was a problem hiding this comment.
Sure does!
I'll rename it.
| * | ||
| * @since 7.1 | ||
| */ | ||
| public abstract class DurationUtil { |
There was a problem hiding this comment.
Yeah... That's exactly why we need a ProperyEditor for Duration.
We can raise an issue for SF.
There is also another problem for XML bean definitions: if we have overloaded setters, there is no guarantee which one would be tried by JavaBeans introspection.
The solution is to not have overloaded setters, but name a new one as setReceiveDuration(Duration).
Makes sense?
The Redis queue components were using the deprecated TimeUnit-based API
Currently both RedisQueueOutboundGateway and RedisQueueInboundGateway receive timeouts are based on millis. Uses should be able to use Duration as well as millis when setting receive timeouts
- Use `Duration` for `receiveTimeout` in `RedisQueueMessageDrivenEndpoin - Refactored docs to meet coding standards - Add `DurationUtil` and string-based `setReceiveTimeout` overloads - This was added because string conversion is required for XML Bean definitions - Rebased
Rename setReceiveTimeout for duration overload to setReceiveDuration
cppwfs
force-pushed
the
SI-redis-duration
branch
from
6c00e1b to
ab47f4a
Compare
2 participants
The Redis queue components were using the deprecated TimeUnit-based API