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.

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

Introduce for SQS.SendMessageBatch #101

Merged
merged 7 commits into from
Jul 15, 2024

Conversation

lolleko
Copy link
Contributor

@lolleko lolleko commented Apr 30, 2024

- Introduces SQS.SendMessageBatch function
- Refactors SQS service to use json instead of xml requests.
- Had to bump local stack image because of localstack/localstack#9610
@lolleko lolleko requested a review from a team as a code owner April 30, 2024 13:26
@lolleko lolleko requested review from mstoykov and oleiade and removed request for a team April 30, 2024 13:26
@CLAassistant
Copy link

CLAassistant commented Apr 30, 2024

CLA assistant check
All committers have signed the CLA.

Lorenz Junglas added 5 commits April 30, 2024 15:27
# Conflicts:
#	dist/aws.js
#	dist/aws.js.map
#	dist/index.js
#	dist/index.js.map
#	dist/sqs.js
#	dist/sqs.js.map
@oleiade
Copy link
Member

oleiade commented May 28, 2024

Hi @lolleko 👋🏻

Apologies for the delay. Thanks a lot for your contribution 🙇🏻

At first glance, this looks good. However, could you please update the PR based on the latest state of main? We've made some changes recently and released a couple of minor releases. Currently, the tests of this PR fail, but I have a hunch this might be the cause.

Furthermore, I'm not an SQS user myself. Is there an advantage to using JSON over XML besides the format itself? Is XML possibly deprecated or considered a legacy format?

# Conflicts:
#	dist/aws.js
#	dist/aws.js.map
#	dist/index.js
#	dist/index.js.map
#	dist/sqs.js
#	dist/sqs.js.map
@lolleko
Copy link
Contributor Author

lolleko commented Jun 14, 2024

Hey @oleiade,

no worries, and also sorry from my side for not picking this up again sooner.

Updated to latest main :)

Furthermore, I'm not an SQS user myself. Is there an advantage to using JSON over XML besides the format itself? Is XML possibly deprecated or considered a legacy format?

TBH for me it was mainly a personal preference, since XML is complex to work with especially in JS/TS.
But I also found this in the AWS docs:

Amazon SQS uses AWS JSON protocol as the transport mechanism for all Amazon SQS APIs on the specified AWS SDK versions. AWS JSON protocol provides a higher throughput, lower latency, and faster application-to-application communication. AWS JSON protocol is more efficient in serialization/deserialization of requests and responses when compared to AWS query protocol. If you still prefer to use the AWS query protocol with SQS APIs, see What languages are supported for AWS JSON protocol used in Amazon SQS APIs? for the AWS SDK versions that support Amazon SQS AWS query protocol.

https://docs.aws.amazon.com/AWSSimpleQueueService/latest/SQSDeveloperGuide/sqs-working-with-apis.html

So not deprecated, but discouraged.

@oleiade
Copy link
Member

oleiade commented Jun 17, 2024

I see, thanks a lot @lolleko for the explanation, that makes sense 🙇🏻

I'll go ahead and make a more thorough review then, and let's get this through the door 🎉

Copy link
Member

@oleiade oleiade left a comment

Choose a reason for hiding this comment

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

Looks good to me indeed 👍🏻 👏🏻 🚀

Will make sure to document it and integrate in the next release which I assume will happen once we have also addressed the other outstanding issue/PR about binary content 🙇🏻

Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the contribution @lolleko 🙇

I will let @oleiade merge it in case there is some order he wants to merge PRs in

@oleiade oleiade merged commit 2119312 into grafana:main Jul 15, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants