Add transports based on Azure PaaS #891
Conversation
Codecov Report
@@ Coverage Diff @@
## master #891 +/- ##
=======================================
Coverage 88.52% 88.52%
=======================================
Files 63 63
Lines 6512 6512
Branches 775 775
=======================================
Hits 5765 5765
Misses 665 665
Partials 82 82
Continue to review full report at Codecov.
|
|
@c-w is there anything remaining from your side? |
|
Hi @auvipy. Thanks for your review! If you're happy to merge this, I'm all for it. (I don't have write access to the repository so one of your maintainers will have to hit the merge button.) |
There was a problem hiding this comment.
I added some comments of minor importance. Nice work @c-w !
kombu/transport/azureservicebus.py
Outdated
| def sbq(self): | ||
| if self._sbq is None: | ||
| self._sbq = ServiceBusService( | ||
| service_namespace=getenv('AZURE_SERVICEBUS_ACCOUNT', |
There was a problem hiding this comment.
I am not sure environment variables should work at this level and I don't see any other transport reading from the environment. The configuration should have been done at a higher level (in this case Celery).
kombu/transport/azureservicebus.py
Outdated
|
|
||
| super(Channel, self).__init__(*args, **kwargs) | ||
|
|
||
| for queue in self.sbq.list_queues(): |
There was a problem hiding this comment.
There is an open issue with AWS SQS transport, not requiring ListQueues permissions. In the proposed fix, a predefined list of queue names is given by configuration. Would that apply here as well?
There was a problem hiding this comment.
Azure ServiceBus has three kinds of permissions: listen, send, and manage (docs). Given that we're creating new queues from the connector, we'll anyways need the manage permissions so might as well use the list queues call here. Thoughts?
kombu/transport/azureservicebus.py
Outdated
| return n | ||
|
|
||
| @property | ||
| def sbq(self): |
There was a problem hiding this comment.
Not sure, but perhaps the abbreviation will be difficult to read for readers unfamiliar with the service?
|
@georgepsarakis Thanks for the comments. Addressed them. Let me know if there's anything else. |
|
please rebase |
This pull request adds two new transport implementations: - `azurestoragequeues` is implemented on top of Azure Storage Queues [1]. This offers a simple but scalable and low-cost PaaS transport for Celery users in Azure. The transport is intended to be used in conjunction with the Azure Block Blob Storage backend [2]. - `azureservicebus` is implemented on top of Azure Service Bus [3] and offers PaaS support for more demanding Celery workloads in Azure. The transport is intended to be used in conjunction with the Azure CosmosDB backend [4]. This pull request was created together with @ankurokok, @dkisselev, @evandropaula, @martinpeck and @michaelperel. [1] https://azure.microsoft.com/en-us/services/storage/queues/ [2] celery/celery#4685 [3] https://azure.microsoft.com/en-us/services/service-bus/ [4] celery/celery#4720
There is test coverage for the transports but the tests require Azure credentials to run (passed via environment variables) so codecov doesn't exercise them.
|
Done, @auvipy. |
This pull request adds two new transport implementations:
azurestoragequeuesis implemented on top of Azure Storage Queues. This offers a simple but scalable and low-cost PaaS transport for Celery users in Azure. The transport is intended to be used in conjunction with the Azure Block Blob Storage backend.azureservicebusis implemented on top of Azure Service Bus and offers PaaS support for more demanding Celery workloads in Azure. The transport is intended to be used in conjunction with the Azure CosmosDB backend.This pull request was created together with @ankurokok, @dkisselev, @evandropaula, @martinpeck and @michaelperel.