Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make routing_nodes an independent metric option in cluster state api #10412

Closed

Conversation

lmenezes
Copy link
Contributor

@lmenezes lmenezes commented Apr 3, 2015

closes #10352

@javanna
Copy link
Member

javanna commented Apr 3, 2015

I like it @lmenezes , thanks! Makes sense to keep bw comp in 1.x and still return routing_nodes too when asking for routing_table, I would change that in master probably as a follow-up. @clintongormley want to double check?

@clintongormley
Copy link

+1 to this change, and yes i'd make it 2.0 only

@lmenezes
Copy link
Contributor Author

lmenezes commented Apr 5, 2015

@clintongormley great! but for 2.0 only you mean making routing_table routing_table only, right? the routing_nodes as an independent metric could go on 1.X, right? :)

@clintongormley
Copy link

@lmenezes thinking about it again... it's such an easy change for users to make in 1.6, that i'd be tempted to just go ahead and do it, ie make the change completely in 1.6.

@lmenezes
Copy link
Contributor Author

lmenezes commented Apr 6, 2015

@clintongormley perfect for me. just thought it could be useful to have a smooth transition, where you could upgrade your nodes and have old behaviour, and then adapt your code on a cluster that already has this change.

anyway, any of the alternatives work for me :)

@javanna
Copy link
Member

javanna commented Apr 7, 2015

Hi @lmenezes I gave a final look at this, I wonder if we should add the same options to the Java api as well. We would need to add a routingNodes flag to ClusterStateRequest and handle bw comp on the wire format using version checks. Thoughts?

@lmenezes
Copy link
Contributor Author

lmenezes commented Apr 7, 2015

@javanna makes sense. Will update the PR with this :)

@lmenezes
Copy link
Contributor Author

lmenezes commented Apr 8, 2015

@javanna so, just had time to look into this... Does it really make sense to separate that for the Java API?

On the REST API, routing_nodes and routing_table are 2 completely different entities(even though both are rendered from the same data) and returning them or not might make difference(bytes sent through the wire, and the process of iterating the routing table to built the json representation for each of them).

On the Java API, the RoutingTable object has to be sent over the wire anyway, and the RoutingNodes is actually volatile and built on demand from the RoutingTable(https://github.com/elastic/elasticsearch/blob/master/src/main/java/org/elasticsearch/cluster/ClusterState.java#L134 and https://github.com/elastic/elasticsearch/blob/master/src/main/java/org/elasticsearch/cluster/ClusterState.java#L226-L232).

So, unless I'm missing something, it would make no difference at all(except from offering the same options on both Java/REST API) being able to control that individually on the Java API, and likely also make it confusing since you cannot really offer RoutingNodes without offering RoutingTable.

Makes sense?

@javanna
Copy link
Member

javanna commented Apr 8, 2015

hey @lmenezes you are right, this makes perfect sense to me. The routing table is what gets serialized over the wire, routing nodes are built on demand when requested first based on the routing table. I will merge this as-is, thanks a lot for looking into this!

javanna pushed a commit to javanna/elasticsearch that referenced this pull request Apr 8, 2015
Cluster state api returns both routing_table and routing_nodes sections whenever routing_table is requested. That is pretty much the same info, just grouped differently. This commit allows to differentiate between the two. Yet, routing_table still returns both for bw comp reasons.

Closes elastic#10352
Closes elastic#10412
@lmenezes
Copy link
Contributor Author

lmenezes commented Apr 8, 2015

@javanna thank you for taking the time to merge this 👍

@javanna javanna closed this in 5fd9aee Apr 8, 2015
@javanna
Copy link
Member

javanna commented Apr 8, 2015

Merged, thanks a lot @lmenezes !

javanna added a commit to javanna/elasticsearch that referenced this pull request Apr 8, 2015
…hrough specific flag

For bacwards compatibility reasons routing_nodes were previously printed out when routing_table was requested, together with the actual routing_table. Now they are printed out only when requests through `routing_nodes` flag.

 Relates to elastic#10412
javanna added a commit that referenced this pull request Apr 8, 2015
…hrough specific flag

For bacwards compatibility reasons routing_nodes were previously printed out when routing_table was requested, together with the actual routing_table. Now they are printed out only when requests through `routing_nodes` flag.

Relates to #10412
Closes #10486
@clintongormley clintongormley changed the title routing_nodes as an independent metric option in cluster state api Make routing_nodes an independent metric option in cluster state api Jun 6, 2015
@clintongormley clintongormley added :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. and removed :Cluster labels Feb 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. >enhancement v1.6.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

separate /_cluster/state routing_table into routing_table and routing_nodes
3 participants