[jaeger-v2] Streamline storage initialization #5171
Conversation
Signed-off-by: Yuri Shkuro <github@ysh.us>
| ext: s, | ||
| storageKind: "memory", | ||
| cfg: s.config.Memory, | ||
| // memory factory does not return an error, so need to wrap it |
There was a problem hiding this comment.
Why not change memory.NewFactoryWithConfig to return the error? It doesn't look like it's used anywhere else, so it shouldn't be a large breaking change.
There was a problem hiding this comment.
Because I hate functions that are declared with errors but always return nil. The caller either has to ignore the error, or it has no easy way of unit testing the error handing path (which will never be used). Lose-lose. I think if the function can never return an error it should never declare it as a possibility (unless you're dealing with an interface where there's no choice).
| for name, b := range s.config.Badger { | ||
| if _, ok := s.factories[name]; ok { | ||
| return fmt.Errorf("duplicate badger storage name %s", name) | ||
| func (s *starter[Config, Factory]) build(ctx context.Context, host component.Host) error { |
There was a problem hiding this comment.
Internal state isn't persisted between method calls so all methods are effectively read-only; we could assert this by using value receivers instead of pointer receivers.
There was a problem hiding this comment.
true, but makes no difference in this context since the code runs exactly once. Don't want to reset the approval just for this fix :-)
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test