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

DISPATCH-2262 - Added an app-level (above Proton) heartbeat feature… #1396

Merged
merged 2 commits into from
Oct 27, 2021

Conversation

ted-ross
Copy link
Member

…that can be used when the Proton heartbeats aren't in effect.

This is a workaround for a possible Proton heartbeat issue.

The heartbeat facility can be used by any endpoint that connects to the router by creating a sender to address '_$qd.edge_heartbeat'. Once this sender is attached, messages must be sent along the link to keep the connection alive. The connection will be closed by the connected router after 8 seconds of no traffic on the heartbeat link.

In this PR, edge routers use this facility to protect their uplink connections to interior routers.

Question: should the timeout be configurable?

…hat can be used when the Proton heartbeats aren't in effect.
Copy link
Contributor

@kgiusti kgiusti left a comment

Choose a reason for hiding this comment

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

Looks good - approved

//
qdr_terminus_t *target = qdr_terminus(0);
qdr_terminus_set_address(target, QD_TERMINUS_HEARTBEAT);
client->endpoint = qdrc_endpoint_create_link_CT(client->core, client->edge_conn, QD_OUTGOING,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this patch, but it probably makes sense to assign a priority to created endpoint links. qdrc_endpoint_create_link_CT() creates all links with priority 4. It probably makes sense to send these heartbeats at a higher priority.

@ted-ross ted-ross merged commit 81725ee into apache:main Oct 27, 2021

fld = qd_compose(QD_PERFORMATIVE_PROPERTIES, fld);
qd_compose_start_list(fld);
qd_compose_insert_int(fld, client->next_msg_id); // message-id
Copy link
Member

Choose a reason for hiding this comment

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

Does this really insert an int? Since int isnt one of the message-id types (string, uuid, binary, ulong).

Comment on lines +148 to +151
qd_composed_field_t *fld = qd_compose(QD_PERFORMATIVE_HEADER, 0);
qd_compose_start_list(fld);
qd_compose_insert_bool(fld, 0); // durable
qd_compose_end_list(fld);
Copy link
Member

Choose a reason for hiding this comment

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

Why not just omit the header? It would be equivalent since durable is false by default.

Comment on lines +159 to +160
qd_composed_field_t *body = qd_compose(QD_PERFORMATIVE_BODY_AMQP_VALUE, 0);
qd_compose_insert_null(body);
Copy link
Member

Choose a reason for hiding this comment

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

Rather than specify a [non-unique] message-id, and also sending an amqp-value+null body, why not just omit the properties section as well and have the amqp-value section contain the count?

Less to encode and smaller to send. Doesnt seem like it is even decoded actually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants