Add heartbeat tune config option #1216 #1215
Conversation
|
After checking the code, it appears the Java client does the following variation of lowest: If both are greater than 0, use the lowest value of the two. That means the only difference between the Java client and If you want I can add that negotiation logic also as an option. I can also limit the options and just go for these three:
Let me know what you prefer. |
|
Please use the same logic as |
|
@lukebakken The point of the PR is to allow the client to choose a behavior. Simply replacing the logic would also introduce a breaking change. If you do not want to add a feature to choose tuning behavior that's fine, then I will close the PR. |
|
For the next major version, this client should be brought in-line with the Java client. Until then, the current behavior is fine. Thanks for looking into it. |
|
8000
I personally liked this idea. We could start from 2-3 possible options, like default(backwards compatible), prefer broker, prefer client, auto. |
|
Well that's a bummer, I can't unlock #1214. I can assign it to the 4.0 release. |
2bb23d6 to
5b5e9a3
Compare
|
@ramunasd @lukebakken Ok, I've updated the PR, now there are 4 options:
The default is set to Let me know what you think. |
43117ac to
dcc5202
Compare
80dd91e to
b5294ed
Compare
| @@ -939,15 +939,32 @@ protected function connection_tune($args) | |||
| $this->frame_max = (int)$v; | |||
| } | |||
|
|
|||
| // @see https://www.rabbitmq.com/heartbeats.html | |||
There was a problem hiding this comment.
I added comments in AMQPConnectionConfig on each constant that explain how it works, so this comment felt like it became redundant. if you prefer to keep it shall I then move it to inside the case AMQPConnectionConfig::HEARTBEAT_TUNE_LEGACY: ? and do you then also want a comment in the other case statements?
There was a problem hiding this comment.
also note that the comment contradictory:
If either value is 0 (see below), the greater value of the two is used
that is the Java client behavior, but the last line states:
For BC, this library opts for disabled heartbeat if client value is 0.
which is the actual implementation, so it only uses the greater value of the two if the broker value is 0
There was a problem hiding this comment.
@ramunasd how would you like me to proceed here?
b5294ed to
9403b26
Compare
9403b26 to
59cabb4
Compare
|
Interesting to note here too maybe, we did some checking on other client libraries we use. Node's amqplib does the same negotiation as the Java client (use the lowest value between client and broker, unless one of them is 0, then take the max, only disable if both are 0). Python's aio-pika always uses the client value and does not do negotiation. The property |
Add a connection configuration option to determine connection heartbeat tuning, implements #1216.