-
-
Notifications
You must be signed in to change notification settings - Fork 927
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
Added global_keyprefix support for pubsub clients #1495
Conversation
What kind of help do you need with the testing? |
@thedrow |
t/unit/transport/test_redis.py
Outdated
channel.active_fanout_queues.add('a') | ||
|
||
channel._subscribe() | ||
mock_execute_command.assert_called() |
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.
Why do we need assert_called if we have assert_called_with?
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.
Removed it.
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.
Just a few small neat-picks.
@@ -217,7 +217,7 @@ def _prefix_args(self, args): | |||
if command in self.PREFIXED_SIMPLE_COMMANDS: | |||
args[0] = self.global_keyprefix + str(args[0]) | |||
|
|||
if command in self.PREFIXED_COMPLEX_COMMANDS.keys(): | |||
elif command in self.PREFIXED_COMPLEX_COMMANDS: |
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.
Remove the line break please. It was there because these conditions where unrelated although you are right, they should be.
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.
fixed it.
the fix from celery/kombu#1495 This fixes the issue where task events from the cloudigrade worker were not seen by the internal celery metrics. High level: With kombu 5.2.3, redis client psubscribes are not subscribed to a channel named with the global_keyprefix that we specified with the broker_transport options. However, task and worker events are keyed with the prefix, so they are never seen by the celery metrics. Removing the global_keyprefix (i.e. user prefix via CLOUDIGRADE_ENVIRONMENT) events now are properly seen, but this won't work with a shared redis instance. With the fix in the PR mentioned above, the global_keyprefix is properly handled with pubsub clients.
the fix from celery/kombu#1495 This fixes the issue where task events from the cloudigrade worker were not seen by the internal celery metrics. High level: With kombu 5.2.3, redis client psubscribes are not subscribed to a channel named with the global_keyprefix that we specified with the broker_transport options. However, task and worker events are keyed with the prefix, so they are never seen by the celery metrics. Removing the global_keyprefix (i.e. user prefix via CLOUDIGRADE_ENVIRONMENT) events now are properly seen, but this won't work with a shared redis instance. With the fix in the PR mentioned above, the global_keyprefix is properly handled with pubsub clients.
This version includes: celery/kombu#1495 which fixes an issue where pubsub clients would not work with messages that include the global_keyprefix.
This version includes: celery/kombu#1495 which fixes an issue where pubsub clients would not work with messages that include the global_keyprefix.
* Added global_keyprefix support for pubsub clients * Added test cases
Fixes #1494
Need help with testcases.