-
-
Notifications
You must be signed in to change notification settings - Fork 928
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 confirm_timeout argument to publish() #2167
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2167 +/- ##
=======================================
Coverage 81.49% 81.49%
=======================================
Files 77 77
Lines 9509 9509
Branches 1148 1148
=======================================
Hits 7749 7749
Misses 1569 1569
Partials 191 191 ☔ View full report in Codecov by Sentry. |
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.
The actual change is never tested @thedrow.
The unit test only covers that the new argument is being passed correctly down the stack, which is excellent. But the functionality of the confirm_timeout
hasn’t really been tested. It doesn’t, for example, test that when the timeout is over, there’s an error, which validates that the new argument actually changes anything.
Lastly, shouldn’t there be a matching change in Celery for using this new flag? Or is it already unpacked as part of an existing flow and depends on the user adding it?
What you are talking about is an integration test, correct? |
Yes, correct :) |
I have written the following integration test but it fails since even the smallest timeout possible is not enough for it to timeout: @pytest.mark.env('py-amqp')
@pytest.mark.flaky(reruns=5, reruns_delay=2)
class test_PyAMQPBasicFunctionality(BasicFunctionality):
def test_publish_confirm_timeout(self, request):
connection = Connection(
hostname=os.environ.get('RABBITMQ_HOST', 'localhost'),
port=os.environ.get('RABBITMQ_5672_TCP', '5672'),
vhost=getattr(
request.config, "slaveinput", {}
).get("slaveid", None),
transport_options={
"confirm_publish": True
}
)
with connection.Producer() as producer:
with pytest.raises(MessageNacked):
producer.publish(
'test',
confirm_timeout=sys.float_info.min
) |
@Nusnus All Celery tests are passing. |
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 have written the following integration test but it fails since even the smallest timeout possible is not enough for it to timeout:
@pytest.mark.env('py-amqp') @pytest.mark.flaky(reruns=5, reruns_delay=2) class test_PyAMQPBasicFunctionality(BasicFunctionality): def test_publish_confirm_timeout(self, request): connection = Connection( hostname=os.environ.get('RABBITMQ_HOST', 'localhost'), port=os.environ.get('RABBITMQ_5672_TCP', '5672'), vhost=getattr( request.config, "slaveinput", {} ).get("slaveid", None), transport_options={ "confirm_publish": True } ) with connection.Producer() as producer: with pytest.raises(MessageNacked): producer.publish( 'test', confirm_timeout=sys.float_info.min )
Is there a way to manually test it maybe?
Also, please fix conflicts and rebase on main
If the integration test doesn't work then it will be hard to reproduce this without an extremely busy production environment. |
@Nusnus Rebased. Please merge once CI is green. |
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.
If the integration test doesn't work then it will be hard to reproduce this without an extremely busy production environment. I'll rebase.
This means we can’t confirm that this fix actually works, but I also understand the difficulty of testing that.
I’ll take your word on it @thedrow then to allow us to move forward.
Approved.
When publishing a message in confirm mode, there was no way to specify the confirmation timeout in
publish()
.This PR adds a new
confirm_timeout
argument to address that.