Skip to content

Conversation

@arnaudmorin
Copy link
Contributor

The AMQP protocol is saying that we should send "two" "heartbeat frames" during the "heartbeat timeout" (see [1] and rabbit implement this in [2]).

The "two" value is the "rate" parameter in the current implementation.

The current implementation was sending only one frame during the "heartbeat timeout", which is wrong.

[1] https://www.amqp.org/specification/0-9-1/amqp-org-download
[2] https://www.rabbitmq.com/heartbeats.html#heartbeats-interval

@pslestang
Copy link

pslestang commented Feb 28, 2023

From my POV there is a main issue in the library implementation. The current implementation is mixing heartbeat (the interval between two tick) and the heartbeat timeout on amqp connection.

py-amqp/amqp/connection.py

Lines 441 to 445 in 9d7b36a

self.send_method(
spec.Connection.TuneOk, argsig,
(self.channel_max, self.frame_max, self.heartbeat),
callback=self._on_tune_sent,
)

As written in rabbitmq documentation "It is important to not confuse the timeout value with the interval one".

The amqp connection should be open with the heartbeat timeout taking care of timeout negotiation with server and then send a tick every heartbeat interval.

Technically it is enough to get the heartbeat timeout and the heartbeat rate to compute the heartbeat interval.

@arnaudmorin
Copy link
Contributor Author

Units are green on my side, is there anything wrong in the github actions?

@arnaudmorin arnaudmorin closed this Mar 9, 2023
@arnaudmorin arnaudmorin reopened this Mar 9, 2023
@auvipy
Copy link
Member

auvipy commented Mar 9, 2023

I restarted

@arnaudmorin
Copy link
Contributor Author

up

@arnaudmorin
Copy link
Contributor Author

recheck

@auvipy
Copy link
Member

auvipy commented Jun 12, 2023

From my POV there is a main issue in the library implementation. The current implementation is mixing heartbeat (the interval between two tick) and the heartbeat timeout on amqp connection.

py-amqp/amqp/connection.py

Lines 441 to 445 in 9d7b36a

self.send_method(
spec.Connection.TuneOk, argsig,
(self.channel_max, self.frame_max, self.heartbeat),
callback=self._on_tune_sent,
)

As written in rabbitmq documentation "It is important to not confuse the timeout value with the interval one".

The amqp connection should be open with the heartbeat timeout taking care of timeout negotiation with server and then send a tick every heartbeat interval.

Technically it is enough to get the heartbeat timeout and the heartbeat rate to compute the heartbeat interval.

@arnaudmorin I would like to know your POV on this

@arnaudmorin
Copy link
Contributor Author

I agree with what Pierre Samuel is explaining, but refactoring this would lead to an API change (the parameter would be renamed / reconfigured), and I dont think this is something we want on py-amqp now.

Eventually this could be done in another change.

@auvipy
Copy link
Member

auvipy commented Jun 13, 2023

can you please try to change the rabbitmq docker image to fix the integration test?

@auvipy
Copy link
Member

auvipy commented Jun 13, 2023

lets see how this #414 go

@auvipy
Copy link
Member

auvipy commented Jun 14, 2023

you can rebase now

This small change in tox.ini allow giving pytests extra arguments, for
e.g.
tox -e pypy-39-unit -- -k test_heartbeat_check_rate_ -s

which allow running only part of the tests (useful for debug purpose).

Signed-off-by: Arnaud Morin <[email protected]>
Change-Id: I535acdb918c6caaae6e6ea4596437076a38f7a59
The AMQP protocol is saying that we should send "two"
"heartbeat frames" during the "heartbeat timeout" (see [1] and rabbit
implement this in [2]).

The "two" value is the "rate" parameter in the current implementation.

The current implementation was sending only one frame during the
"heartbeat timeout", which is wrong.

[1] https://www.amqp.org/specification/0-9-1/amqp-org-download
[2] https://www.rabbitmq.com/heartbeats.html#heartbeats-interval

Signed-off-by: Arnaud Morin <[email protected]>
@auvipy auvipy merged commit a92dd03 into celery:main Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants