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-1103 - Added code to retry failed auto links using the core … #366

Closed
wants to merge 3 commits into from

Conversation

ganeshmurthy
Copy link
Contributor

…timeer API

Copy link
Member

@ted-ross ted-ross left a comment

Choose a reason for hiding this comment

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

See comments. I think the modularity and function-abstraction can be further improved in this feature. It will make route_control easier to maintain and extend in the future.

* @param core Pointer to the core object returned by qd_core()
* @param context - the auto link that needs to be activated
*/
void qdr_route_attempt_auto_link_CT(qdr_core_t *core, void *context);
Copy link
Member

Choose a reason for hiding this comment

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

This is not part of the route_control API. It is simply an internal helper within the module. It should be removed from the header file (API) and made static in the source file.

@@ -1329,6 +1329,9 @@ static void qdr_connection_closed_CT(qdr_core_t *core, qdr_action_t *action, boo
while (link_ref) {
qdr_link_t *link = link_ref->link;

if (link->auto_link && link->auto_link->retry_timer)
qdr_core_timer_cancel_CT(core, link->auto_link->retry_timer);
Copy link
Member

Choose a reason for hiding this comment

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

The fact that there is a retry-timer in the auto-link is an implementation detail that should not be visible at this level. It would be better to have a call in the route_control module for "auto_link_connection_closed" or similar.

// The auto link has failed. Periodically retry setting up the auto link until
// it succeeds.
//
qdr_route_retry_auto_link_CT(core, link);
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here: At this location in the architecture, the fact that there is a retry is abstracted. This call should be "auto_link_detached" or similar. Let the route_control module be responsible for what actions it takes when an auto-link is detached by the peer.

@asfgit asfgit closed this in c65fbc9 Aug 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants