Remove NodeBuilder #15354
Remove NodeBuilder #15354
Conversation
The NodeBuilder is currently used to construct a Node. However, this is really just yet-another-builder that wraps around a Settings.Builder witha couple convenience methods. But there are very few uses of these convenience methods. This change removes NodeBuilder, in favor of just using the Node constructor.
| @@ -31,11 +31,11 @@ | |||
| public static void main(String[] args) { | |||
| long OPERATIONS = SizeValue.parseSizeValue("300k").singles(); | |||
|
|
|||
| Node node = NodeBuilder.nodeBuilder().node(); | |||
| Node node = new Node(Settings.EMPTY); | |||
|
Left some minor comments (missing |
|
+1 |
|
I'm in favor of this change, but not in favor of making a change that will break compilation of client code in a minor release (this is currently labeled v2.2.0). |
|
@jasontedor NodeBuilder has always been an internal API. The fact that some users (who embed ES) use it instead of the Node constructor is not a reason we should hold back internal refactoring. Users who embed ES are advanced enough that they can update to use the Node constructor. |
|
I removed the 2.2 label, and opened an issue (#15383) to change the docs so constructing an in memory Node is never recommended. |
Since we are in java you have to recompile your client code anyway so the compiler will tell you what's going on. This is a great way to move on with APIs - when I am in the situation I was always happy to see a hard break to make the changes early and often. |
|
@rjernst can we have a note in the breaking changes docs please |
is this necessary? It's unsupported really and a Java API |
|
Well. It has always been documented in the Java API doc. So that makes it a bit "official" IMO. |
|
It's how we now officially communicate on how to start a client. https://www.elastic.co/guide/en/elasticsearch/client/java-api/current/node-client.html +1 on clear messaging as to why we are changing it - to me, breaking changes feel natural. |
|
+1 to freely break this in 2.x and explain why in the breaking changes docs: javac makes it completely clear to these very expert users on upgrade that they need to use the |
I think this is the source of where people have different feelings. These are not expert settings - it's what we recommend on our website and many Java people use. I'm totally +1 with changing that (on 3.0 I would go as far as not treating this as a client at all). We should clear document this, explain why and offer people an alternative. |
|
I think this is missing in 5.0 API breaking changes. https://www.elastic.co/guide/en/elasticsearch/reference/current/breaking-changes-5.0.html |
True. @rjernst could you add to the breaking changes please? |
|
Added: |
The NodeBuilder is currently used to construct a Node. However, this is
really just yet-another-builder that wraps around a Settings.Builder
witha couple convenience methods. But there are very few uses of these
convenience methods. This change removes NodeBuilder, in favor of just
using the Node constructor.