The implementation of the type ServiceRegistry is generally clear and readable. It follows good practices such as using interfaces for service definitions and logging operations. Here are some observations and minor suggestions for improvement:
Use map[reflect.Type]bool to keep information about whether a service is already added to the registry. Use a slice []Service to maintain the order of services as they are added and should be started. This way, it will not be necessary to touch both structures (map and slice) in all methods. The map will be used only when a service is registered to check whether it already exists.
There is confusing logging in the method StartAll. If a service can't be started, we log a message:
s.logger.Errorf("Cannot start service %s: %s", typ, err)
But at the end, we log:
s.logger.Debug("All services started.")
which is not true because not all services are started.
Consider returning an error from the method StartAll when a service can't be started. I see that all services are started in the same order they are registered, so I guess following services might depend on the previous ones. If one service can't be started and another service depends on it, then it might be better to return an error. Is it okay to continue if some services are not working? Is it possible that the node will not work properly?
Move the logic for pausing a service before stopping it inside the service. If it is important for some services to be paused before being stopped, it might be better to move this logic inside the Stop method of each service. This prevents the need for everyone using this service to duplicate the same logic. Also, what will happen if the user of the service forgets to pause it before stopping it? It is good for the service to have an exposed method Pause, and users should pause the service if they want. However, if the pause is mainly needed before stopping, then this logic should be moved inside the service and not duplicated.
Is it really important to run the services in the order in which they are registered? If that is true, it means some services depend on others. What will happen if we pause a service on which other services, which can't be paused, depend?
If the order of starting services is important and some services must be started before others, then why do we stop the services in the same order? Shouldn't we stop them in the reverse order (first stop services that are not dependencies for others)?
The Get method has an inconsistency between its documentation and its implementation. The documentation is:
// Get retrieves a service and stores a reference to it in the passed in `srvc`
func (s *ServiceRegistry) Get(srvc interface{}) Service {
But this is not true. The method returns a Service and does not assign the service inside srvc.
Issue summary
The implementation of the type ServiceRegistry is generally clear and readable. It follows good practices such as using interfaces for service definitions and logging operations. Here are some observations and minor suggestions for improvement:
Use map[reflect.Type]bool to keep information about whether a service is already added to the registry. Use a slice []Service to maintain the order of services as they are added and should be started. This way, it will not be necessary to touch both structures (map and slice) in all methods. The map will be used only when a service is registered to check whether it already exists.
There is confusing logging in the method StartAll. If a service can't be started, we log a message:
But at the end, we log:
which is not true because not all services are started.
Consider returning an error from the method StartAll when a service can't be started. I see that all services are started in the same order they are registered, so I guess following services might depend on the previous ones. If one service can't be started and another service depends on it, then it might be better to return an error. Is it okay to continue if some services are not working? Is it possible that the node will not work properly?
Move the logic for pausing a service before stopping it inside the service. If it is important for some services to be paused before being stopped, it might be better to move this logic inside the Stop method of each service. This prevents the need for everyone using this service to duplicate the same logic. Also, what will happen if the user of the service forgets to pause it before stopping it? It is good for the service to have an exposed method Pause, and users should pause the service if they want. However, if the pause is mainly needed before stopping, then this logic should be moved inside the service and not duplicated.
Is it really important to run the services in the order in which they are registered? If that is true, it means some services depend on others. What will happen if we pause a service on which other services, which can't be paused, depend?
If the order of starting services is important and some services must be started before others, then why do we stop the services in the same order? Shouldn't we stop them in the reverse order (first stop services that are not dependencies for others)?
The Get method has an inconsistency between its documentation and its implementation. The documentation is:
But this is not true. The method returns a Service and does not assign the service inside srvc.