Remove common.Daemon from Monitor interface #4084
Conversation
| rpcConfig *config.RPC | ||
| tlsFactory encryption.TLSConfigProvider | ||
| dc *dynamicconfig.Collection | ||
| monitor *monitor |
There was a problem hiding this comment.
Why change this to the struct type instead of keep the interface type?
There was a problem hiding this comment.
To have access to the Start/Stop methods which are no longer in the interface
The idea is that users of the interface should not need to know about Start/Stop because every time they use the object it should be ready because of the fx lifecycle management. Additionally, implementations shouldn't need these methods because they should manage the lifecycle themselves via the fx Hooks they add. This will make it easier to replace our existing monitor implementation as well. Some implementations may want to use the context from fx.OnStart as well. |
I don't see why exposing Start/Stop is in any way bad. In fact, I think exposing gives more flexibility to the user to work with the daemon. Is there any issue y calling Start/Stop multiple times? They should be idempotent. In any case, I don't have much context on this, so no strong feelings about it. |
What changed?
Why?
The Start and Stop methods of the Monitor shouldn't be required because it should be up to implementors how a monitor is started and stopped. Currently, our RingPop monitor already does this by using fx, so we don't even want users of the package to call Start and Stop in an adhoc manner.
How did you test it?
I stepped through with a debugger and verified that the call to Start in the worker service was a no-op because start is already called as part of the fx lifecycle. I could not verify the Stop logic because I don't think Stop is even called when we shut down the server.
Potential risks
Is hotfix candidate?