[Docs] Add migration notes for the high-level rest client #25911
Conversation
There was a problem hiding this comment.
It isn't really a Create, Read, Update, or Delete operation so maybe it should be in its own test just for naming's sake.
There was a problem hiding this comment.
It doesn't use Java serialization, right? I mean, it uses a our home grown binary serialization.
There was a problem hiding this comment.
Absolutely... I had trouble to find the right phrase and in the end it ended with something wrong :/
There was a problem hiding this comment.
I wouldn't say it is perfect - it is acceptable, but it ties the user application quite tightly to Elasticsearch.
There was a problem hiding this comment.
I agree - I changed perfect for acceptable. Let me know if you think the whole sentence should be reworded.
There was a problem hiding this comment.
It has more than two transitive dependencies because it has Elasticsearch's dependencies too.
There was a problem hiding this comment.
Oh right - I replaced two transitive dependencies by dependencies as I find it simpler to understand.
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
thanks for working on this, I left some comments
docs/java-api/client.asciidoc
Outdated
There was a problem hiding this comment.
which sends HTTP requests instead of using the transport layer.
docs/java-api/client.asciidoc
Outdated
There was a problem hiding this comment.
Now is actually a good time to deprecate it :) Feel free to make that change already here or mention that it is deprecated in 6.0
docs/java-api/index.asciidoc
Outdated
There was a problem hiding this comment.
I wonder what you mean by compatible here. There are breaking changes, and that's why we are working on this page?
There was a problem hiding this comment.
Yeah, I agree it's not clear. I rephrased that, I just wanted to say that you can use the client to connect to a cluster in version 5.6.0+.
There was a problem hiding this comment.
I wonder if this warning is needed. Isn't this kind of implicit given that you never specify the cluster name when specifying the hosts?
There was a problem hiding this comment.
By writing this, I wanted to "promote" what I think would be a good practice... on the other side, nothing prevent the user to configure multiple hosts from different clusters :( Let's remove this.
There was a problem hiding this comment.
gut feeling: these motivations are more for a blogpost that for our docs maybe? I would rather focus on how to migrate here.
There was a problem hiding this comment.
I don't know - I added this to add some context around why it's good to migrate, since it's easy to miss a blog post. But I can remove this whole section if you really feel it should not be here.
There was a problem hiding this comment.
I like it. Maybe it should be in a blog post as well, but I like explaining it here as too.
|
Thanks @javanna for your review, it is very helpful. I tried to complete the documentation following your indications. I'm pretty sure it still need some work but it gets closer, I hope. |
docs/java-api/index.asciidoc
Outdated
There was a problem hiding this comment.
to replace the TransportClient (remove "the usage")
There was a problem hiding this comment.
This may make people think the the high level client doesn't depend on Elasticsearch, also we don't mention security and wanting to have REST a single entrypoing for a cluster. Shall we link https://www.elastic.co/blog/state-of-the-official-elasticsearch-java-clients that mentions all that and keep this explanation a bit shorter?
We could say:
The TransportClient has been part of Elasticsearch since its very first commit. It is a special client as it uses the transport protocol to communicate with Elasticsearch, which causes compatibility problems if the client is not on the same version as the Elasticsearch instances it talks to.
We released a low-level REST client in 2016, which is based on the well known Apache HTTP client and it allows to communicate with an Elasticsearch cluster in any version using HTTP. On top of that we released the high-level REST client which is based on the low-level client but takes care of request marshalling and response un-marshalling.
Have a look at (here the link) to know more about these changes.
There was a problem hiding this comment.
This may make people think the the high level client doesn't depend on Elasticsearch,
Right, this can be a bit confusing and it's hard to resume all the motivations in a single paragraph.
I just borrow your text and let's go!
There was a problem hiding this comment.
we should probably more clear on whether it's fine to talk from 5.6.0 to 6.0.0. I guess it is, but new apis won't be known to the client this way. Also breaking changes to the API may cause unexpected results.
There was a problem hiding this comment.
I wonder if we have to put the whole maven dependency here, especially given that it's the old one. Maybe we can just say replace the o.e.client:transport dependency with... ?
There was a problem hiding this comment.
can we invert the two bullet points, leaving request builders first?
There was a problem hiding this comment.
I would move this paragraph to the end of the page
There was a problem hiding this comment.
Makes sense. Should I also move "Checking Cluster Health using the Low-Level REST Client" down?
There was a problem hiding this comment.
and can therefore be used to call the APIs that are not yet supported by the high level client.
There was a problem hiding this comment.
The RestHighLevelClient supports the same request and response object as the TransportClient, but exposes slightly different methods to send the requests.
javanna
changed the title
javanna
changed the title
|
Thanks @javanna - changes have been merged. I'll merge this unless you have more suggestions. |
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
I left a couple of comments, but this looks great, thanks a lot
There was a problem hiding this comment.
I would rephrase this:
The Java High Level Rest Client requires Java 1.8 and depends on Elasticsearch core. The client version is the same as the Elasticsearch version that the client was developed for. The High Level Client is backwards compatible but can only communicate with Elasticsearch version 5.5 and onwards. The High Level Client is forward compatible as well, meaning that it supports communicating with a later version of Elasticsearch than the one it was developed for. It is recommended to upgrade the High Level Client when upgrading the Elasticsearch cluster to a new major version, as REST API breaking changes may cause unexpected results, and newly added APIs will only be supported by the newer version of the client. The client should be updated last, once all of the nodes in the cluster have been upgraded.
I am now thinking if this should go somewhere else rather than in the migration guide.
There was a problem hiding this comment.
Awesome. I added your text in a "Compatibility" section in the usage page. I created a link in the migration guide to this page.
There was a problem hiding this comment.
typically initialized as follows
There was a problem hiding this comment.
oops sorry, it shouldn't have been reverted
6 participants
This PR adds some notes in the Java API and Reference documentation that mention the existence of the Java High Level Rest Client.
It also adds a migration page in the high level client documentation. It is definitively perfectible but that's a start.