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

SQS - Gossip not supported #1337

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

galCohen88
Copy link
Contributor

add gossip not supported exception

@galCohen88 galCohen88 changed the title add gossip not supported exception Add gossip not supported exception Apr 22, 2021
@@ -314,6 +322,10 @@ def _new_queue(self, queue, **kwargs):
"""Ensure a queue with given name exists in SQS."""
if not isinstance(queue, str):
return queue

if queue.endswith('reply-celery-pidbox'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right place to put this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

considering the following stack trace, yes:
File "kombu/transport/virtual/base.py", line 528, in queue_declare self._new_queue(queue, **kwargs) File "kombu/transport/SQS.py", line 233, in _new_queue raise UndefinedQueueException

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd guess that @thedrow was questioning whether kombu should be aware of things like this queue suffix which are specific to celery. It feels like a special case. Is there any other property of the queue args or channel instance we can check to reach the same conclusion? Maybe we instead need some property on the class like does_ping_mean_anything_for_this_type_of_channel() -> bool?

Bumping this since it's on the in progress list for 5.1.0 still.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maybe-sybr I think this is a relatively narrow use-case, where the generic use-case you are suggesting is too broad. I don't see a place where this property will be used besides with SQS broker.
About the transport layer to be aware of the context - I think it's inevitable. The alternative is making Celery aware of SQS specifics, which is the exact same problem

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Transports have capabilities as far as I recall.
We can add a new one instead and let celery handle raising the error.
I think that would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thedrow Can you give me some hints around where would you like me to implement this? I don't have an idea where to start

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default_transport_capabilities = Implements(
asynchronous=False,
exchange_type=frozenset(['direct', 'topic', 'fanout', 'headers']),
heartbeats=False,
)

implements = virtual.Transport.implements.extend(
asynchronous=True,
exchange_type=frozenset(['direct']),
)

I'd assume a new key in the default_transport_capabilities set to True so we don't break any existing working behaviours, having that same key set to False in the SQS.Transport, and then some logic in celery to avoid the ping/gossip behaviour if the transport has that implements key set to something falsey? Perhaps something like implements['multiple_queues']?

Alternatively, maybe the fact that SQS.Transport only supports the 'direct' exchange type is a valid proxy for this limitation already? We could potentially do something like:

if transport.implements.get('exchange_type', set()) - {'direct'}:
  # support more than just direct queues -> attempt gossip
  do_gossip_stuff()

or something like that? Maybe that check could be a fallback for if a new implements key which gets added per the paragraph above is missing (to handle celery upgrades with kombu having been held back)?

caveat emptor: I have literally never touched this code so I'm making this all up on the fly. If @thedrow or @auvipy have any further input, I'd trust them more than me :) Hope this helps, and thanks for the work and thought on this so far.

@galCohen88 galCohen88 changed the title Add gossip not supported exception SQS - Gossip not supported Apr 22, 2021
@thedrow thedrow added this to In progress in Celery 5.1.0 via automation Apr 22, 2021
@thedrow thedrow added this to the 5.1.0 milestone Apr 22, 2021
@thedrow thedrow moved this from In progress to Postponed in Celery 5.1.0 May 23, 2021
@auvipy auvipy modified the milestones: 5.1.0, 6.0 Sep 10, 2021
@auvipy auvipy modified the milestones: 6.0, 5.3 Oct 20, 2021
@auvipy auvipy modified the milestones: 5.3, 5.3.x Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Celery 5.1.0
  
Postponed
Development

Successfully merging this pull request may close these issues.

None yet

4 participants