Conversation
|
@copilot please review |
|
@rustyeddy I've opened a new pull request, #35, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the MQTT broker lifecycle management in the Otto framework by moving broker startup/shutdown logic from the OttO struct into the messanger package, introducing lazy initialization in GetMessanger(), and implementing failover handling when a broker is already running.
Key Changes:
- Centralized broker lifecycle management using a global
shutdownvariable inmessanger/mqtt_broker.go - Refactored
GetMessanger()to perform lazy initialization with configuration priority: config → environment variables → default "otto" broker - Simplified
OttO.Init()by delegating messanger creation toGetMessanger()and removed thebrokerShutdownfield
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| otto.go | Removed broker startup logic from Init() and brokerShutdown field; simplified Stop() to call messanger.StopMQTTBroker() |
| otto_test.go | Commented out assertions for removed brokerShutdown field |
| messanger/mqtt_broker.go | Added global shutdown variable and StopMQTTBroker() function for centralized broker lifecycle |
| messanger/messanger.go | Refactored GetMessanger() with lazy initialization and environment variable support; changed NewMessanger() parameter from id to broker |
| messanger/mqtt.go | Changed default credentials from "otto"/"otto123" to empty strings |
| messanger/messanger_test.go | Updated test expectations to match new behavior where unknown IDs create default MQTT messangers |
| config.go | New file embedding messanger.Config for top-level configuration |
| .gitignore | Added coverage.* pattern to ignore coverage output files |
| if o.Messanger != nil { | ||
| o.Messanger.Close() | ||
| } | ||
|
|
||
| if o.brokerShutdown != nil { | ||
| _ = o.brokerShutdown(context.Background()) | ||
| messanger.StopMQTTBroker(context.Background()) |
There was a problem hiding this comment.
The StopMQTTBroker is called unconditionally in the Stop() method even when a Messanger might be using a remote broker (not the embedded one). This could cause issues if:
- The global
shutdownfunction was set by a different component - Multiple OttO instances are running
- The Messanger is using a remote broker
Consider tracking whether this OttO instance started the broker before attempting to stop it, or handle errors from StopMQTTBroker more explicitly.
| func NewMessanger(broker string) (m Messanger) { | ||
|
|
||
| switch broker { | ||
| case "none": | ||
| m = NewMessangerLocal(id) | ||
| case "mqtt": | ||
| m = NewMessangerMQTT(id, messangerConfig.Broker) | ||
| case "local": | ||
| msgr = NewMessangerLocal(broker) | ||
|
|
||
| case "otto": | ||
| _, err := StartMQTTBroker(context.Background()) | ||
| if err != nil { | ||
| slog.Error("Failed to start embedded MQTT broker", "error", err) | ||
| return nil | ||
| } | ||
| m = NewMessangerMQTT(id, messangerConfig.Broker) | ||
| msgr = NewMessangerMQTT(broker, broker) | ||
|
|
||
| default: | ||
| slog.Error("Unknown messanger ID", "id", id) | ||
| return nil | ||
| msgr = NewMessangerMQTT(broker, broker) |
There was a problem hiding this comment.
The NewMessanger function signature has changed from taking an id parameter to taking a broker parameter, but this is semantically confusing. In line 132, NewMessangerLocal(broker) is called where broker is "none", but "none" is being used as an ID, not a broker address. Similarly, in line 140, NewMessangerMQTT(broker, broker) is called where the first parameter should be an ID but is receiving "otto" (the broker type). This parameter naming and usage is inconsistent and could lead to confusion.
| _, err := StartMQTTBroker(context.Background()) | ||
| if err != nil { | ||
| slog.Error("Failed to start embedded MQTT broker", "error", err) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The NewMessanger function discards the shutdown function returned by StartMQTTBroker at line 135, which means there's no way to gracefully shutdown the broker started by this function. This differs from GetMessanger which properly stores it in the global shutdown variable. This creates an inconsistency where brokers started via NewMessanger("otto") cannot be cleanly shut down.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@rustyeddy I've opened a new pull request, #37, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@rustyeddy I've opened a new pull request, #38, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
Changes have been merged. |
Handle starting the broker and handling fail over in the event a broker is already running.