Discovery: HTTP client for communicating with Discovery Service #2711
Conversation
discovery/api/v1/client/interface.go
Outdated
| ) | ||
|
|
||
| // Invoker is the interface for the client that invokes the remote Discovery Service. | ||
| type Invoker interface { |
There was a problem hiding this comment.
Why the interface? Do you expect to mock it for other tests? Most http clients are just structs
There was a problem hiding this comment.
It does eliminate the service implementation (signatures for a service struct would be the same)
There was a problem hiding this comment.
Yes, I mocked it in tests. The simple function signatures of the interface makes it mocking easier (over having to spin up an actual HTTP client or mocking the HTTP calls).
| if err != nil { | ||
| return nil, nil, fmt.Errorf("failed to invoke remote Discovery Service (url=%s): %w", serviceEndpointURL, err) | ||
| } | ||
| defer httpResponse.Body.Close() |
There was a problem hiding this comment.
Is this needed when a ReadAll is done?
There was a problem hiding this comment.
In case ReadAll() fails. But then, the connection might not be reused anyways;
... It is the caller's responsibility to
// close Body. The default HTTP client's Transport may not
// reuse HTTP/1.x "keep-alive" TCP connections if the Body is
// not read to completion and closed.I tend to always make sure I close external resources, what do you think?
discovery/api/v1/client/http.go
Outdated
| } | ||
| } | ||
|
|
||
| var _ Invoker = &HTTPInvoker{} |
There was a problem hiding this comment.
we have 8 HTTPClients in the other APIs. Is there a reason to deviate from our own convention here?
There was a problem hiding this comment.
I wanted to have an interface, which shouldn't be named Client since it's already in a package named client. Naming it HTTPClient makes naming the implementation hard, since it would be something like DefaultHTTPClient. But I guess that's the least of all evils, although it includes the package name. But consistency is probably the most important here (again).
There was a problem hiding this comment.
DefaultHTTPClient could also be httpClient. IDEs will complain about returning an unexported type from an exported function, but it works just fine if you only intend to use the interface methods outside of this package
No description provided.