Better categorization for transport actions #7105
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Field field = transportService.getClass().getDeclaredField("serverHandlers"); | ||
| field.setAccessible(true); | ||
| ImmutableMap<String, TransportRequestHandler> requestHandlers = (ImmutableMap<String, TransportRequestHandler>) field.get(transportService); | ||
|
|
There was a problem hiding this comment.
note the reflection shenanigans here...we need this unless we move this bw comp test to the org.elasticsearch.transport package, I didn't do it yet since I saw that all the bw comp tests are under the same package for now.
| @@ -428,4 +548,191 @@ public void cancel() { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| private static ImmutableBiMap<String, String> createActionNamesMap() { | |||
There was a problem hiding this comment.
(putting the comment here... not sure where else to put it)
would love to see all these mappings extracted to separate class (say ActionNames) and keep the service as clean as possible... these methods can move there as well:
public TransportRequestHandler handler(String action, Version version) { ... }
public String action(String action, Version version) { ... }
|
Pushed a new commit that addresses your feedback @uboness |
|
LGTM.. would be good to have another pair of eyes on this one (@kimchy ?) |
|
LGTM |
Our transport relies on action names that tell what we need to do with each message received and sent on any node, together with the content of the request itself.
The action names could use a better categorization and more consistent naming though, the following are the categories introduced with this commit:
- indices: for all the apis that execute against indices
- admin: for the apis that allow to perform administration tasks against indices
- data: for the apis that are about data
- read: apis that read data
- write: apis that write data
- benchmark: apis that run benchmarks
- cluster: for all the cluster apis
- admin: for the cluster apis that allow to perform administration tasks
- monitor: for the cluster apis that allow to monitor the system
- internal: for all the internal actions that are used from node to node but not directly exposed to users
The change is applied in a backwards compatible manner: we keep the mapping old-to-new action name around, and when receiving a message, depending on the version of the node we receive it from, we use the received action name or we convert it to the previous version (old to new if version < 1.4). When sending a message, depending on the version of the node we talk to, we use the updated action or we convert it to the previous version (new to old if version < 1.4).
For the cases where we don't know the version of the node we talk to, namely unicast ping, transport client nodes info and transport client sniff mode (which calls cluster state), we just use a lower bound for the version, thus we will always use the old action name, which can be understood by both old nodes and new nodes.
Added test that enforces known updated categories for transport action names and test that verifies all action names have a pre 1.4 version for bw compatibility
Added backwards compatibility tests for unicast and transport client in sniff mode, the one for the ordinary transport client (that call nodes info) is implicit as it's used all the time in our bw comp tests.
Added also backwards comp test that sends an empty message to any of the registered transport handler exposed by older nodes and verifies that what gets back is not ActionNotFoundTransportException, which would mean that there is a problem in the actions mappings.
Added TestCluster#getClusterName abstract method and allow to retrieve externalTransportAddress and internalCluster from CompositeTestCluster.
…version logic to it and added specific tests for it
javanna
added a commit
that referenced
this pull request
Aug 4, 2014
Our transport relies on action names that tell what we need to do with each message received and sent on any node, together with the content of the request itself.
The action names could use a better categorization and more consistent naming though, the following are the categories introduced with this commit:
- indices: for all the apis that execute against indices
- admin: for the apis that allow to perform administration tasks against indices
- data: for the apis that are about data
- read: apis that read data
- write: apis that write data
- benchmark: apis that run benchmarks
- cluster: for all the cluster apis
- admin: for the cluster apis that allow to perform administration tasks
- monitor: for the cluster apis that allow to monitor the system
- internal: for all the internal actions that are used from node to node but not directly exposed to users
The change is applied in a backwards compatible manner: we keep the mapping old-to-new action name around, and when receiving a message, depending on the version of the node we receive it from, we use the received action name or we convert it to the previous version (old to new if version < 1.4). When sending a message, depending on the version of the node we talk to, we use the updated action or we convert it to the previous version (new to old if version < 1.4).
For the cases where we don't know the version of the node we talk to, namely unicast ping, transport client nodes info and transport client sniff mode (which calls cluster state), we just use a lower bound for the version, thus we will always use the old action name, which can be understood by both old nodes and new nodes.
Added test that enforces known updated categories for transport action names and test that verifies all action names have a pre 1.4 version for bw compatibility
Added backwards compatibility tests for unicast and transport client in sniff mode, the one for the ordinary transport client (which calls nodes info) is implicit as it's used all the time in our bw comp tests.
Added also backwards comp test that sends an empty message to any of the registered transport handler exposed by older nodes and verifies that what gets back is not ActionNotFoundTransportException, which would mean that there is a problem in the actions mappings.
Added TestCluster#getClusterName abstract method and allow to retrieve externalTransportAddress and internalCluster from CompositeTestCluster.
Closes #7105
javanna
added a commit
that referenced
this pull request
Sep 8, 2014
Our transport relies on action names that tell what we need to do with each message received and sent on any node, together with the content of the request itself.
The action names could use a better categorization and more consistent naming though, the following are the categories introduced with this commit:
- indices: for all the apis that execute against indices
- admin: for the apis that allow to perform administration tasks against indices
- data: for the apis that are about data
- read: apis that read data
- write: apis that write data
- benchmark: apis that run benchmarks
- cluster: for all the cluster apis
- admin: for the cluster apis that allow to perform administration tasks
- monitor: for the cluster apis that allow to monitor the system
- internal: for all the internal actions that are used from node to node but not directly exposed to users
The change is applied in a backwards compatible manner: we keep the mapping old-to-new action name around, and when receiving a message, depending on the version of the node we receive it from, we use the received action name or we convert it to the previous version (old to new if version < 1.4). When sending a message, depending on the version of the node we talk to, we use the updated action or we convert it to the previous version (new to old if version < 1.4).
For the cases where we don't know the version of the node we talk to, namely unicast ping, transport client nodes info and transport client sniff mode (which calls cluster state), we just use a lower bound for the version, thus we will always use the old action name, which can be understood by both old nodes and new nodes.
Added test that enforces known updated categories for transport action names and test that verifies all action names have a pre 1.4 version for bw compatibility
Added backwards compatibility tests for unicast and transport client in sniff mode, the one for the ordinary transport client (which calls nodes info) is implicit as it's used all the time in our bw comp tests.
Added also backwards comp test that sends an empty message to any of the registered transport handler exposed by older nodes and verifies that what gets back is not ActionNotFoundTransportException, which would mean that there is a problem in the actions mappings.
Added TestCluster#getClusterName abstract method and allow to retrieve externalTransportAddress and internalCluster from CompositeTestCluster.
Closes #7105
clintongormley
changed the title
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Dec 3, 2014
The bwc layer added with elastic#7105 is not needed in master as a full cluster restart will be required, thus from 2.0 on the only supported action names are compliant to the defined conventions and don't need to be converted to the old format
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Jan 27, 2015
When we renamed all of the transport actions in elastic#7105, shard started and failed were flipped around by mistake. This didn't cause any actual problem as the change was made in a backwards compatible manner. That said the action names ended up being wrong (just a naming problem, meaning that started == failed and failed ==started) and we have to fix it, still maintaining backwards compatibility.
javanna
added a commit
that referenced
this pull request
Jan 27, 2015
When we renamed all of the transport actions in #7105, shard started and failed were flipped around by mistake. This didn't cause any actual problem as the change was made in a backwards compatible manner. That said the action names ended up being wrong (just a naming problem, meaning that started == failed and failed ==started) and we have to fix it, still maintaining backwards compatibility. Closes #9440
clintongormley
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Our transport relies on action names that tell what we need to do with each message received and sent on any node, together with the content of the request itself.
The action names could use a better categorization and more consistent naming though, the following are the categories introduced with this commit:
indices: for all the apis that execute against indicesadmin: for the apis that allow to perform administration tasks against indicesdata: for the apis that are about dataread: apis that read datawrite: apis that write databenchmark: apis that run benchmarkscluster: for all the cluster apisadmin: for the cluster apis that allow to perform administration tasksmonitor: for the cluster apis that allow to monitor the systeminternal: for all the internal actions that are used from node to node but not directly exposed to usersThe change is applied in a backwards compatible manner: we keep the mapping old-to-new action name around, and when receiving a message, depending on the version of the node we receive it from, we use the received action name or we convert it to the previous version (old to new if version < 1.4). When sending a message, depending on the version of the node we talk to, we use the updated action or we convert it to the previous version (new to old if version < 1.4).
For the cases where we don't know the version of the node we talk to, namely unicast ping, transport client nodes info and transport client sniff mode (which calls cluster state), we just use a lower bound for the version, thus we will always use the old action name, which can be understood by both old nodes and new nodes.
Added test that enforces known updated categories for transport action names and test that verifies all action names have a pre 1.4 version for bw compatibility
Added backwards compatibility tests for unicast and transport client in sniff mode, the one for the ordinary transport client (that call nodes info) is implicit as it's used all the time in our bw comp tests.
Added also backwards comp test that sends an empty message to any of the registered transport handler exposed by older nodes and verifies that what gets back is not
ActionNotFoundTransportException, which would mean that there is a problem in the actions mappings.Added
TestCluster#getClusterNameabstract method and allow to retrieve externalTransportAddress and internalCluster fromCompositeTestCluster.