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

Randomise reconnect period #104

Merged
merged 1 commit into from
Sep 11, 2024
Merged

Randomise reconnect period #104

merged 1 commit into from
Sep 11, 2024

Conversation

Steve-Mcl
Copy link
Contributor

closes #103

Description

To alleviate stresses on broker, a variation in the reconnect timing is introduced

The exact value may wish to be tweaked but I went with 2s either side of the original period (5s) i.e. between 3s and 7s

Additionally, this time period avoids the reconnect time period of the device agent. see FlowFuse/device-agent#315

For reference: MQTTJS defaults to reconnecting at 1s

Related Issue(s)

#103

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Includes a DB migration? -> add the area:migration label

@Steve-Mcl Steve-Mcl requested a review from knolleary September 9, 2024 07:56
@knolleary
Copy link
Member

As with the device agent - does this cover the initial connection as well?

@Steve-Mcl
Copy link
Contributor Author

As with the device agent - does this cover the initial connection as well?

No, only reconnect.

The MQTTJS lib itself does not support such a "connecttion delay".

For us to introduce this in the nodes, there would be quite some restructuring needed:

  • mqtt.connect is operated upon node creation (in the node constructor, non-async)
    • this creates the client object
    • subsequent code within all 3 node-types expect the client object to be ready for calls to subscribe and for hooking up events.
  • To differentiate simple node-deploys and node-red starting for the first time is not without complication

In short:

To support an initial connection delay without setting the node constructor async would require some refactoring to how we initialise all 3 nodes so as to defer subscriptions etc to the connected event or some other means.

Question:

Off the top of your head, are there any consequences to making a node-red nodes constructor async so that we can simplify implementation of a delay (by way of awaiting a timeout promise)

Or perhaps you can think of a alternative initialisation pattern for the 3 nodes?

@knolleary
Copy link
Member

Lets skip the startup connect delay - too much refactoring needed for not a lot of benefit,

@knolleary knolleary merged commit f814664 into main Sep 11, 2024
6 checks passed
@knolleary knolleary deleted the 103-reconnect-delay branch September 11, 2024 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Project Nodes reconnect delay
2 participants