Implement an interface for the elasticsearch client#2974
Implement an interface for the elasticsearch client#2974jalvz merged 2 commits intoelastic:masterfrom
Conversation
| return nil, nil, err | ||
| } | ||
| // NewVersionedClient returns the right elasticsearch client for the current Stack version, as an interface | ||
| func NewVersionedClient(user, pwd string, addresses []string, transport http.RoundTripper) (Client, error) { |
There was a problem hiding this comment.
In my opinion the main driver for this should be compatibility of APM Server with Elasticsearch, for the latest minor of the last major and the first minor of the next major version.
I don't see how this is achieved with this PR. The version.GetDefaultVersion() returns the defaultBeatVersion which will be the same for the running APM Server, independent of the Elasticsearch instances it is talking to.
In my opinion what would need to happen is that for every given ES address a request would be required to fetch the Elasticsearch version and then depending on that a client can be constructed. This would still open a couple of questions, like how to behave if the instances are running different versions, how to behave on upgrades, etc.
The second motivation for abstracting away the original elasticsearch client to keep backports to a minimum is not a strong motivation in my opinion. If the v7/v8 clients behave differently we will also need to implement both behaviours with this abstraction. I don't see a big difference effort wise, compared to changing the logic in the backport, except that the import would need to be updated for the version.
There was a problem hiding this comment.
Forgot to mention that the actual logic to address that compatibility issue is out the scope of this PR, since it is not so urgent.
This PR lays out the ground for it, I created a follow up ticket #2975
Codecov Report
@@ Coverage Diff @@
## master #2974 +/- ##
==========================================
- Coverage 78.74% 78.29% -0.45%
==========================================
Files 84 85 +1
Lines 4380 4414 +34
==========================================
+ Hits 3449 3456 +7
- Misses 931 958 +27
|
|
@simitt what changes are you requesting here? |
|
I requested changes together with my PR review comment #2974 (comment). Haven't come around to another round of review yet. |
| // under the License. | ||
|
|
||
| package test | ||
| package estest |
There was a problem hiding this comment.
Why the rename? The test package is already inside the elasticsearch path.
There was a problem hiding this comment.
It was an easy way to avoid an unneeded import alias (eg f26d615#diff-2b990608de1ddbd3f6ec60d8b0e68f25R33)
It also seems to be conventional (eg. see httptest inside http I assume for the same reason https://golang.org/pkg/net/http/httptest/), but I can revert if you feel strong about it.
I pushed changes for the other comments
elasticsearch/config.go
Outdated
| Addresses: addresses, | ||
| Transport: transport, | ||
| }) | ||
| } |
There was a problem hiding this comment.
newClient methods should be moved to client.go.
elasticsearch/config.go
Outdated
| proxy func(*http.Request) (*url.URL, error) | ||
| dialer transport.Dialer | ||
| tlsDialer transport.Dialer | ||
| } |
There was a problem hiding this comment.
The proxy, dialer and tlsDialer are only used for creating the http.Transport. How about not introducing another struct but just a function httpTransport(config *Config) (*http.Transport, error)?
There was a problem hiding this comment.
it was also for addresses, but i changed it anyways
| tlsDialer, err := transport.TLSDialer(dialer, tlsConfig, cfg.Timeout) | ||
| return dialer, tlsDialer, err | ||
| c, err := newV7Client(user, pwd, addresses, transport) | ||
| return clientV7{c}, err |
There was a problem hiding this comment.
Can you add a test for the versioned client? The easiest thing for now would probably be to expect a v8 client on master and a v7 one on 7.x.
d92ed4e to
f04e7af
Compare
…rsion differences. The objective is to keep master and release branches as close as possible, facilitate backports over time, and isolate API breaking changes in one single file when they come around; allowing the rest of the code to be blissfully ignorant of the Elasticsearch API. Fixes elastic#2967
f04e7af to
f503a47
Compare
The objective is to keep master and release branches as close as possible, facilitate backports over time, and isolate API breaking changes in one single file when they come around; allowing the rest of the code to be blissfully ignorant of the Elasticsearch API.
This presents a couple of minor drawbacks: requiring an extra dependency (2 versions of the client) and some code duplication.
While this is not pretty, in the face of pragmatism I think it is OK: the weight of the extra dependency is negligible, and the duplicated bits are trivial.
The resulting code is still concise IMO, and we will eventually have to duplicate some bits any ways.
We rather lay the ground for it now and maintain the rest of the code oblivious to elasticsearch versions.
Fixes #2967