-
-
Notifications
You must be signed in to change notification settings - Fork 945
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
SQS: Support Async JSON SQS Protocol & Message Attributes #2226
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2226 +/- ##
==========================================
+ Coverage 81.54% 81.57% +0.02%
==========================================
Files 77 77
Lines 9517 9531 +14
Branches 1154 1158 +4
==========================================
+ Hits 7761 7775 +14
Misses 1564 1564
Partials 192 192 ☔ View full report in Codecov by Sentry. |
5a2b480
to
dc78076
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for noticing and handling this! after initial review, my understanding is this PR support both old and new protocols. since this is a substantial change, should this be included in a major version? as it seems mostly compatibility code.
@auvipy I do not think this needs to be a new major version. This doesn't add new support so much as fix support for something that broke along the way. It's all compatible code and there are no changes required for consumers of celery/kombu for this change. Eventually we may want to rip out the old API, but that is a future PR and would probably be a major. |
oh yeah right you are. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reviewing the changes, I think we can merge it for v5.5rc release as it keeps both bc and add forward compat as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hfern can you share update on the following please?
The only things that broke were compound data types for ReceiveMessage -- those are attribute names. Other things did not break because the code path for post-process message deletion is through the synchronous branch, which uses standard boto and not the async request builder (kombu.transport.SQS.Channel.basic_ack). Nevertheless, I fixed all the parameters in the async client.
@auvipy In hindsight, my message was ambiguous -- sorry! The "things that broke" I mentioned was what led me to making this PR. They were already broken. This PR does not break anything -- it actually fixes the breakages from earlier. |
@auvipy 👋 Hi there! There is no ticket for this issue -- I found this bug myself and thought I'd fix it.
Adds support for consuming botocore SQS
json
models in the SQS async code. This only affects consumer code.Several parameters are supplied incorrectly as they have changed in the
query
tojson
botocore change. If you have a newer version of botocore, you will get thejson
service model instead of the oldquery
model. I found several places where kombu was generating calls to the newjson
API (indicated by the x-amz-json-1.0 content-type) but providing parameter names consistent with thequery
protocol.This PR makes kombu aware of both the
json
andquery
protocol query names and provide them both. When the AWS request is later constructed from the service model, we apply protocol-specific and common parameters. Unfortunately, AWS has not maintained consistency in the parameter names, so I manually reviewed each API call and supplied both.The only things that broke were compound data types for ReceiveMessage -- those are attribute names. Other things did not break because the code path for post-process message deletion is through the synchronous branch, which uses standard boto and not the async request builder (
kombu.transport.SQS.Channel.basic_ack
). Nevertheless, I fixed all the parameters in the async client.Logged request after PR (note new usage of
AttributeNames
instead ofAttributeName.1
):This code supporting both protocols can be cleaned up if Kombu is willing to require
botocore>=1.34.90
, which was released in April 2024 (boto/botocore/pull/3165). It can take quite a while to move forward the boto3/botocore/moto ecosystem, so Kombu may want to give consumers some time before dropping old botocores. Either way, this PR would be required.As I fixed this to get support for AttributeNames back, I also added support for customizing fetched attributes via
transport_options.fetch_message_attributes
. Now you can supply["All"]
to get better diagnostics from SQS. I added documentation for this.