Add a delay before reconnecting to MQTT #161
Conversation
| @@ -508,6 +509,12 @@ func main() { | |||
| Value: true, | |||
| Hidden: true, | |||
| }), | |||
| altsrc.NewDurationFlag(&cli.DurationFlag{ | |||
There was a problem hiding this comment.
It is pretty confusing that you cannot use --mqtt-reconnect-delay with simple numeric value. I was expecting that it should work and value would be considered in seconds.
I think that we should emphasize in Usage: that argument should be something like 10s (not only 10 or "10").
There was a problem hiding this comment.
I think that we should also improve Usage text of other occurrences of DurationFlag.
There was a problem hiding this comment.
Internally, the DurationFlag uses time.ParseDuration to parse the value. That function does not make assumptions about default duration unit if the unit is omitted. The error returned by an invalid value for the flag does not make a suggestion about how to specify a valid format. That error comes from the cli package though; we can't change it.
These are hidden flags though; their Usage won't be visible to a user in the --help output. I changed the hidden value to true to see what the usage text looks like:
--mqtt-reconnect-delay DURATION Sets the time to wait before attempting to reconnect to DURATION (default: 0s)
The default value there hints at using a duration unit, so if we do ever make these visible flags, the usage does suggest to the user how to specify a correct value.
There was a problem hiding this comment.
Oh, you are right, when the flag is hidden, then the usage could not visible.
| @@ -99,6 +99,18 @@ func NewMQTTTransport(clientID string, brokers []string, tlsConfig *tls.Config) | |||
| t.events <- TransporterEventDisconnected | |||
| }) | |||
|
|
|||
| opts.SetReconnectingHandler(func(c mqtt.Client, co *mqtt.ClientOptions) { | |||
There was a problem hiding this comment.
When --mqtt-connect-retry is used, then this handler function is also used. I hope that we could improve messaging in this case. When yggd is not able to reconnect, then some exponential backoff algorithm is used for computing next attempt. It would be nice to see something like, when reconnecting failed: reconnecting failed, next reconnect attempt in X seconds. I don't see where to print such message. I also do not know if it is possible to get information about value of delay computed by exponential backoff algorithm.
There was a problem hiding this comment.
Yea, the MQTT client package does the backing off itself. We just get a handler that's called before the reconnect attempt happens. I don't think the value is exported by the package at all.
There was a problem hiding this comment.
I just noticed that the delay is printed, when MQTT_DEBUG is set.
https://pkg.go.dev/github.com/eclipse/paho.mqtt.golang#ClientOptions.SetConnectRetryInterval |
subpop
force-pushed
the
sleep-before-reconnect
branch
from
4f337f4 to
888f319
Compare
Make sure the values of the mqtt-* flags are pulled from the CLI context into the DefaultConfig. Signed-off-by: Link Dupont <link@sub-pop.net>
Add a new hidden flag: --mqtt-reconnect-delay. This flag will sleep for the given duration before attempting to reconnect to the MQTT broker. This only works if --mqtt-auto-reconnect is enabled (it is true by default). Signed-off-by: Link Dupont <link@sub-pop.net>
subpop
force-pushed
the
sleep-before-reconnect
branch
from
888f319 to
d15eb02
Compare
| @@ -508,6 +509,12 @@ func main() { | |||
| Value: true, | |||
| Hidden: true, | |||
| }), | |||
| altsrc.NewDurationFlag(&cli.DurationFlag{ | |||
There was a problem hiding this comment.
Oh, you are right, when the flag is hidden, then the usage could not visible.
| @@ -99,6 +99,18 @@ func NewMQTTTransport(clientID string, brokers []string, tlsConfig *tls.Config) | |||
| t.events <- TransporterEventDisconnected | |||
| }) | |||
|
|
|||
| opts.SetReconnectingHandler(func(c mqtt.Client, co *mqtt.ClientOptions) { | |||
There was a problem hiding this comment.
I just noticed that the delay is printed, when MQTT_DEBUG is set.
Add a new hidden flag: --mqtt-reconnect-delay. This flag will sleep for the given duration before attempting to reconnect to the MQTT broker. This only works if --mqtt-auto-reconnect is enabled (it is true by default).
I also noticed that the previously added MQTT flags were not set into the DefaultConfig struct, so their values when read from the struct were always the zero value. I fixed this in a separate commit on this same PR.