-
-
Notifications
You must be signed in to change notification settings - Fork 930
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
Fix 1063 on 4.x branch #1262
Fix 1063 on 4.x branch #1262
Conversation
… missing from Redis. This should fix the test from earlier commit.
Unable to build successfully because of two issues, both unrelated to my changes:
Seems like it's using the test_redis.Client object which doesn't have The error is:
|
So why are you backporting this? |
We just upgraded to the 4.x branch of Celery and noticed this issue. Since 4.x is LTS, we want it fixed there. |
@thedrow you locked #1063 ( #1063 (comment) ) and asked for a PR that reproduces and fixes it. But the PR already exists and is this one. |
in any case this should target master branch. in kombu we do not have proper 4.6.x branch like in celery |
except OperationalError as exc: | ||
# Fixes https://github.com/celery/kombu/issues/1063 | ||
|
||
if not exc.args and not is_no_route_error_for_reply_celery_pidbox(exc.args[0]): |
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.
So what exactly happens here?
Why reporting this error is good enough?
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.
We're not raising the error if it's got a certain message.
@hsophie-sf You should target master anyway. I've done so for you. |
I'm pretty convinced that this is the wrong place to fix this. @lambacck Your analysis is 100% correct though. |
… error should NOT be raised now, after the fix.
CI is failing due to unrelated reason now: https://github.com/hsophie-sf/kombu/runs/2667819031?check_suite_focus=true on test |
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.
please rebase
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.
I agree with @thedrow feedback:
I'm pretty convinced that this is the wrong place to fix this. @lambacck Your analysis is 100% correct though.
This is a good workaround but the actual fix should be in the Redis transport as it only happens there right?
Also, I don't understand why this error happens yet.
Moreover, I miss kind of warning when the exception is ignored.
5.2.0 |
Fixes #1063
Details on #1063 (comment)
First commit is a failing test which is fixed in second commit.