feat: Add Queue (SQS)
#114
Conversation
✅ Deploy Preview for stelvio-docs-main ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
michal-stlv
left a comment
michal-stlv
left a comment
There was a problem hiding this comment.
This misses the main thing - handler. Function ( and later container etc.) which will handles messages from queue. Also DL queue setting etc
…nto feature/sqs
docs/guides/queues.md
Outdated
| - `sqs:SendMessage` - Send messages to the queue | ||
| - `sqs:ReceiveMessage` - Read messages from the queue | ||
| - `sqs:DeleteMessage` - Delete processed messages | ||
| - `sqs:GetQueueAttributes` - Read queue metadata | ||
| - `sqs:GetQueueUrl` - Retrieve the queue URL |
There was a problem hiding this comment.
When linking we link queue to lambda that will send message to the queue. Does this lambda need Receive, Delete permssion? (Maybe there is a use case, like clearing message or checking if some message is already there. Just double checking if it makes sense as a default). Also get url is useless as url is in envars?
There was a problem hiding this comment.
-
GetQueueUrlshould be left for convenience -
ReceiveMessage,DeleteMessagecould be parameterized.. wdyt?
There was a problem hiding this comment.
ReceiveMessage, DeleteMessage could be parameterized.. wdyt?
How? there's a way to customize link already, user can do that if he wants. I'm there was idea to have extra methods on Link but since all components use same class it won't fly
docs/guides/queues.md
Outdated
| |----------------------------|-------------------------------------------------------------| | ||
| | **Process queue messages** | Use `queue.subscribe()` - creates Lambda triggered by queue | | ||
| | **Send messages to queue** | Use `links=[queue]` - grants permissions to send messages | | ||
| | **Both read and write** | Use both subscription AND link to another queue | |
There was a problem hiding this comment.
What's case for this? Sounds confusign, one lambda would send message to the queue and also process it? how'd that work in stelvio code?
There was a problem hiding this comment.
one lambda would receive a message and send a message to another queue
There was a problem hiding this comment.
hmm ok, wasn't clear to me from docs. maybe we can expand it there or change wording.
There was a problem hiding this comment.
Clearer with this?
| **Pipeline (read → write)** | Subscribe to one queue, link to another for forwarding |
stelvio/aws/queue.py
Outdated
| delay: int = 0 | ||
| visibility_timeout: int = 30 | ||
| retention: int = 345600 # 4 days in seconds (default) | ||
| dlq: str | DlqConfig | DlqConfigDict | None = None |
There was a problem hiding this comment.
I'm wondering if we even want to have DlqConfig?
It has just two properties and creates hassle for user (create dict or instance) and for us (all that handling code to parse it etc.).
Maybe simple and better UX would be just to have dlq: Queue | str | None and then additional param dlq_retry: int param? If documented it's all good and simple?
There was a problem hiding this comment.
I'd rather have the (admittedly verbose) types and config parsing. with customize we already run into liniting warnings with lots of parameters and other things which are not that nice... we should soon refactor all params to such classes/dicts, but we need to discuss first
There was a problem hiding this comment.
we should soon refactor all params to such classes/dicts, but we need to discuss first
what are those cases? when component have more than 2-3 param we have *Config for it. Number of params is not problem for init only for other methods/functions.
My main problem is that if user want's to just have dlq he needs to construct dict or import config. instead of just assignign it. wondering if we want to allow also Queue instead of of DlqConfig... or that's too much? e.g.
dlq: Queue | str | DlqConfig | DlqConfigDict | None = NoneThere was a problem hiding this comment.
having said that subscribe method has one or two params to go :)
|
Hey Bas @sebst, one more thing I noticed - SQS EventSourceMapping supports filter_criteria which lets you filter which messages trigger the Lambda based on message attributes or body content. This is useful when you want different handlers for different message types without filtering in code. Currently subscribe() doesn't expose this. Not a blocker for this PR - the implementation works fine without it. But wanted to flag it so we can add it in a follow-up. We support this on dynamo table and I'm working on it for SNS too. We'd wrap the AWS complexity (like we do for DynamoDB streams) so users get bit simpler API. And we'd handle converting that to the full AWS filter_criteria JSON structure internally. Thoughts? Fine to merge as-is and add later, or do you want to include it now? (I'd still like to have it in 0.7) |
No description provided.