Add SQS module @SnsNotificationMessage batch messages support #1133
Conversation
maxjiang153
requested review from
maciejwalkowiak,
MatejNedic and
tomazfernandes
as code owners
maxjiang153
force-pushed
the
gh-1129
branch
3 times, most recently
from
09ca31f to
d1cdf70
Compare
github-actions
bot
added
the
type: documentation
|
@tomazfernandes I've added the doc description for this, please check it out. And if everything is okay, I'll rebase it into a single commit. |
docs/src/main/asciidoc/sqs.adoc
Outdated
|
|
||
| The @SnsNotificationMessage annotation can be used for both single and batch message processing. | ||
|
|
||
| For single message processing, you can use the @SnsNotificationMessage annotation as follows: |
There was a problem hiding this comment.
We avoid using you and and use a more formal tone instead.
So maybe something like the @SnsNotificationMessage annotation can be used as follows:?
docs/src/main/asciidoc/sqs.adoc
Outdated
| @@ -626,6 +631,16 @@ public void listen(@SnsNotificationMessage Pojo pojo) { | |||
| } | |||
| ---- | |||
|
|
|||
| For batch message processing, you can use the @SnsNotificationMessage annotation with a List<Pojo> parameter: | |||
There was a problem hiding this comment.
Same here about using you
There was a problem hiding this comment.
done, check it out again plz.
There was a problem hiding this comment.
Thanks for the PR @maxjiang153!
Looks good overall, left a couple of questions.
| * SNS Message wrapper. | ||
| */ | ||
| public record SnsMessageWrapper(String subject, Object message) { | ||
| static Type getResolvedType(Class<?> targetClass, @Nullable Object conversionHint) { |
There was a problem hiding this comment.
Why is this static and not private?
| @@ -77,7 +105,7 @@ public Object fromMessage(Message<?> message, Class<?> targetClass, @Nullable Ob | |||
| ? ((SmartMessageConverter) this.payloadConverter).fromMessage(genericMessage, targetClass, | |||
| conversionHint) | |||
| : this.payloadConverter.fromMessage(genericMessage, targetClass); | |||
| return new SnsMessageWrapper(jsonNode.path("Subject").asText(), convertedMessage); | |||
| return convertedMessage; | |||
There was a problem hiding this comment.
Why remove the Wrapper?
While I understand we're not currently using the subject, I think it might be helpful if we eventually want to allow retrieving it.
Wdyt?
There was a problem hiding this comment.
The reason is because when NotificationMessageArgumentResolver tries to resolving the messages, it might be a single message or a message list. So it will be more tricky to keep the wrapper when processing this situation.
So that's the reason I removed the wrapper.
|
Hey @maxjiang153, just a ping here to maybe try to merge this before the next version is out. Thanks! |
|
@tomazfernandes Hi, sorry for the late response. I was busy with other stuff. I've already updated this PR and check it again pls. |
|
Same thing here @maxjiang153, I added some polishing to this PR to expedite merging: #1191 Thanks again for the PR and looking forward to more! |
📢 Type of change
📜 Description
see: #1129
💡 Motivation and Context
💚 How did you test it?
📝 Checklist
🔮 Next steps