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

Draft - AWS - SQS support #2402

Closed
wants to merge 15 commits into from
Closed

Conversation

holomekc
Copy link

@holomekc holomekc commented Dec 6, 2023

Hi I started with an implementation. There is still a lot of work to do. So help would be welcome. I created the pr, because maybe reviews, questions, help is easier this way.

Progress:

  • SQS
    • Sending messages
    • Sending batch messages
    • Outgoing channel
    • Get queue url from name
    • Create queue if not exists
    • Receive messages
    • Incoming channel
    • Delete / Confirm messages
    • Delete / Confirm batch messages
    • Testing
      • partially. I started with a WeldTestBase and added some tests for consuming and sending messages. There is still a lot to do.
    • More config options
    • Logging
    • Error handling
    • OpenTelemetry (prepared)
    • Health
    • Graceful shutdown
      • Partially. For the tests I was forced to add some logic here already. It is not done by far. I just made sure that the consumer is closed properly. It does not wait for the processing of the messages though. Probably more is still open.
  • SNS
    • TODO

…A lot of things are missing though: logging, finalize tracing, error handling, retry, config. After that polling could be implemented. I am afraid that the tracing cannot be implemented properly due to reactive, but we will see. Tests are missing completely.
…cannot handle some things. The reason is that the metadata is not available but instead is created by this impl and more for providing information from lib to user. So I moved it to configuration. This is not really nice, because I need to configure 4 maps. The config impl. here cannot handle that, so manual parsing is necessary. This is strange. Maybe this needs to change.

Receive message implemented sort of. Deletion / Confirmation is missing. Should be the same as Sending. Also with batching.
… bug fixes and config adjustments to make it work. Start with graceful shutdown, which was needed for the tests to prevent localstack from connection loss. Well only needed for faster tests with one localstack instance. New test module to provide common aws specific logs. Except the WeldTestBase, which seems to be a duplicated multiple times.
…uses a custom deserializer, which is then registered via Identifier annotation. I am not sure if this is worth it. In theory deserialization could be added in the invoked method anyway. Same for serialization. Although it is nice to have an included solution the overhead here is quite high.
@holomekc holomekc mentioned this pull request Dec 6, 2023
@holomekc
Copy link
Author

holomekc commented Dec 6, 2023

Some questions.

  • client configuration. Although I can describe some of the settings, it is very difficult to cover the complete AWS SDK config. Especially the credentials provider contain so many ways to configure them. I have the feeling I would reimplement the quarkiverse solution for SQS. I am not sure if this is wise. Any ideas?
  • delete batching. I am not sure regarding the emitter approach.
  • (de)serialization. I am not sure if this is worth it. I checked the JMS impl. and although it is nice, it is kind of complicated to configure for the deserialization, because I cannot access the class, which is used at the method, which will be invoked. If this would be accessible, then it would be easier and less configuration would be needed. This way a user could use a String and then deserialize later manually anyway.

…ckson is more forgiving and can serialize a string to a string, but jakarta crashes in this case. This broke the tests.
@ozangunalp
Copy link
Collaborator

Thanks for this, I'll give it a spin as soon as possible and make some remarks.
In the meantime there is now a connector contributor guide if you want to check out : https://smallrye.io/smallrye-reactive-messaging/latest/concepts/contributing-connectors

@holomekc
Copy link
Author

Hi @ozangunalp ,

Thx for the feedback. Yes I noticed the guide. I read it partially already. I think I covered most things already by accident. Well or by checking some other connector implementations. I will recheck to make sure that I am not missing something.

A huge topic is the configuration of the AWS SDK client. I did not tackle that yet, because I am not sure what the best approach is.

@holomekc
Copy link
Author

Hi Team,

I stop this contribution. I have no energy to continue it. Sorry for that.

Br,
Chris

@holomekc holomekc closed this Feb 26, 2024
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.

2 participants