Skip to content
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

amqp: heartbeat support #2676

Merged
merged 2 commits into from May 2, 2019
Merged

amqp: heartbeat support #2676

merged 2 commits into from May 2, 2019

Conversation

nobles
Copy link
Contributor

@nobles nobles commented Apr 12, 2019

Users can provide heartbeat in their configuration, that is negotiated with the server. In case heartbeat is enabled, amqp destination will send heartbeats periodically.

Heartbeat is measured in seconds. Default value is zero.

Example configuration:

    destination { amqp(vhost("/")
                  exchange("logs")
                  body("hello world")
                  heartbeat(10)
                  username(guest) password(guest)); };

Fixes: #189

@kira-syslogng
Copy link
Contributor

This user does not have permission to start the build. Can one of the admins verify this patch and start the build?
(admin: you have the next options (make sure you checked the code):
"ok to test" to accept this pull request (and further changes) for testing
"test this please" for a one time test run
do nothing -> CI won't start)

1 similar comment
@kira-syslogng
Copy link
Contributor

This user does not have permission to start the build. Can one of the admins verify this patch and start the build?
(admin: you have the next options (make sure you checked the code):
"ok to test" to accept this pull request (and further changes) for testing
"test this please" for a one time test run
do nothing -> CI won't start)

@furiel
Copy link
Collaborator

furiel commented Apr 12, 2019

@kira-syslogng ok to test

@kira-syslogng
Copy link
Contributor

Build SUCCESS

@nobles nobles force-pushed the amqp-heartbeat branch 2 times, most recently from 61de053 to 6d48377 Compare April 12, 2019 16:35
@kira-syslogng
Copy link
Contributor

Build FAILURE

@furiel
Copy link
Collaborator

furiel commented Apr 12, 2019

@kira-syslogng retest this please

@kira-syslogng
Copy link
Contributor

Build SUCCESS

modules/afamqp/afamqp.c Outdated Show resolved Hide resolved
modules/afamqp/afamqp.c Show resolved Hide resolved
modules/afamqp/afamqp.c Show resolved Hide resolved
@kira-syslogng
Copy link
Contributor

Build FAILURE

@nobles
Copy link
Contributor Author

nobles commented Apr 17, 2019

@kira-syslogng retest this please

1 similar comment
@furiel
Copy link
Collaborator

furiel commented Apr 17, 2019

@kira-syslogng retest this please

@kira-syslogng
Copy link
Contributor

Build SUCCESS

Kokan
Kokan previously approved these changes Apr 17, 2019
Copy link
Collaborator

@Kokan Kokan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for fixing one of the oldest issue/feature request 🥇

@gaborznagy gaborznagy self-requested a review April 24, 2019 08:40
@gaborznagy gaborznagy added the user-visible-feature User visible feature label Apr 25, 2019
modules/afamqp/afamqp.c Show resolved Hide resolved
modules/afamqp/afamqp.c Show resolved Hide resolved
break;

default:
g_assert_not_reached();
return FALSE;
}

self->heartbeat = amqp_get_heartbeat(self->conn);

msg_debug("Amqp heartbeat negotiation",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for the documentation:
As I read the docs, heartbeat can only be turned off, if BOTH sides has a heartbeat value of 0.
https://www.rabbitmq.com/heartbeats.html

Heartbeats can be disabled by setting the timeout interval to 0 on both client and server ends.

On code side we have this debug message, so there is nothing more to do, it's just worth to mention in the docs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UPDATE: I've tried this out and this statement seems invalid.
What I've concluded is that generally the negotiated heartbeat interval will be the smaller configured one.
About disabling heartbeating: Client controls enable/disable, therefore even if the server side has an explicit 0 for heartbeat, a client can override this. If the client has a 0, it means disable heartbeat.
(tested with RabbitMQ 3.6.10 server and client 0.8.0 versions).

Users can provide heartbeat in their configuration,
that is negotiated with the server. In case heartbeat is enabled,
amqp destination will send heartbeats periodically.

Signed-off-by: Terez Nemes <terez.noble@gmail.com>
In case of read error from server during heartbeat (for example when
server misses out heartbeats), we tear down the connection.

Signed-off-by: Terez Nemes <terez.noble@gmail.com>
@nobles
Copy link
Contributor Author

nobles commented Apr 26, 2019

I needed to force push because of conflict

@kira-syslogng
Copy link
Contributor

Build SUCCESS

Copy link
Collaborator

@gaborznagy gaborznagy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve from my side.

I would like to update the PR description emphasizing that the heartbeat interval is configured, not the timeout.

@gaborznagy gaborznagy merged commit 616b0ec into syslog-ng:master May 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user-visible-feature User visible feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

heartbeat support for AMQP
5 participants