SQS: avoid excessive GetQueueURL calls by using cached queue url #1621
Conversation
sparrowt
force-pushed
the
cache-queue-url-issue-1618
branch
2 times, most recently
from
f794969
to
625af1a
Compare
|
Hi @auvipy, thanks for looking at my PR - aside from coverage are you happy with the caching change in principle? I haven't added any new methods or logical code paths, only made a small modification to one existing path - are these core methods not already covered? Is there a way for me to access the coverage results which https://github.com/celery/kombu/actions/runs/3533431074/jobs/6067061888 seems to be recording? Thanks in advance. P.S. I noticed 3 of the CI runs failed, but none looked to be related to this code change, are they expected? |
|
to be honest, the actual change is hard to review, due to lots of renaming. can you please add tests? I am in favour of caching if it doesn't break anything, rather improve excessive calls to GetQURL |
|
This would be easier to review if it included just the changes required to avoid excessive GetQueueURL calls. @sparrowt Any chance you'll refactor this PR and break it down into several PRs? |
|
@sparrowt it would be great if you can revisit this |
|
Hi folks, sorry for the delayed response - I deliberately made this PR with 2 separate commits, the first of which is a minimal fix for the problem, then the second which renames things which appeared to be confusingly named (whilst I was trying to figure out how everything worked). Would you still prefer the 2nd commit to be removed to a separate PR potentially, or are you happy to review it commit by commit? Regarding testing, I assume |
`_get_from_sqs` was unnecessarily calling `get_queue_url` every time even though the only place which calls `_get_from_sqs` (that is `_get_async`) actually already knows the queue URL. This change avoids hundreds of `GetQueueUrl` AWS API calls per hour when using this SQS backend with celery. Also `connection` is set by the one-and-only caller (and `queue` is actually the queue name string now anyway so couldn't ever have `.connection`) so remove the None default and unused fallback code.
It seems that prior to 129a9e4 it returned a queue object but this is no longer the case so update comments variable names accordingly to make it clearer. Also remove the incorrect fallback which cannot be correct any more given the return value has to be the queue URL which must be a string.
sparrowt
force-pushed
the
cache-queue-url-issue-1618
branch
2 times, most recently
from
70035a7
to
75b7ea3
Compare
This key code path (which as far as I can see is the main route when using celery with SQS) was missing test coverage. This test adds coverage for: `_get_bulk_async` -> `_get_async` -> `_get_from_sqs`
sparrowt
force-pushed
the
cache-queue-url-issue-1618
branch
from
75b7ea3
to
7926ff5
Compare
|
I've added coverage for the existing core code path which was missing it (and also rebased my PR), how's that? :) |
|
I've restarted the CI |
auvipy
added
PR Type: Enhancement
Component: Amazon SQS Broker
and removed
Status: Needs Testcase
labels
|
thank you |
The first commit fixes #1618.
The second clarifies some comments & variable names which I found confusing while trying to figure out what was going on.