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

Better categorization for transport actions #7105

Closed

Conversation

Projects
None yet
4 participants
@javanna
Copy link
Member

commented Jul 31, 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 (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.

@javanna javanna self-assigned this Jul 31, 2014

@javanna

View changes

src/test/java/org/elasticsearch/bwcompat/ActionNamesBackwardsCompatibilityTest.java Outdated
Field field = transportService.getClass().getDeclaredField("serverHandlers");
field.setAccessible(true);
ImmutableMap<String, TransportRequestHandler> requestHandlers = (ImmutableMap<String, TransportRequestHandler>) field.get(transportService);

This comment has been minimized.

Copy link
@javanna

javanna Jul 31, 2014

Author Member

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.

@javanna javanna added the review label Jul 31, 2014

@uboness

View changes

src/main/java/org/elasticsearch/transport/TransportService.java Outdated
@@ -428,4 +548,191 @@ public void cancel() {
}
}
}

private static ImmutableBiMap<String, String> createActionNamesMap() {

This comment has been minimized.

Copy link
@uboness

uboness Jul 31, 2014

Contributor

(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) { ... }
@javanna

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2014

Pushed a new commit that addresses your feedback @uboness

@uboness

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2014

LGTM.. would be good to have another pair of eyes on this one (@kimchy ?)

@kimchy

This comment has been minimized.

Copy link
Member

commented Aug 4, 2014

LGTM

javanna added some commits Jul 10, 2014

Transport: better categorization for transport actions
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.
moved action names mapping to separate ActionNames class, moved also …
…version logic to it and added specific tests for it

@javanna javanna removed the review label Aug 4, 2014

javanna added a commit that referenced this pull request Aug 4, 2014

Transport: better categorization for transport actions
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 javanna closed this in d2fea53 Aug 4, 2014

javanna added a commit that referenced this pull request Sep 8, 2014

Transport: better categorization for transport actions
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 clintongormley changed the title Transport: better categorization for transport actions Internal: Better categorization for transport actions Sep 8, 2014

javanna added a commit to javanna/elasticsearch that referenced this pull request Dec 3, 2014

[TEST] remove action names bwc layer
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 that referenced this pull request Dec 3, 2014

[TEST] remove action names bwc layer
The bwc layer added with #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

Closes #8758

javanna added a commit to javanna/elasticsearch that referenced this pull request Jan 27, 2015

Internal: fix shard state tranport action names
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

Internal: fix shard state tranport action names
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

javanna added a commit that referenced this pull request Jan 27, 2015

Internal: fix shard state tranport action names
When we renamed all of the transport actions in #7105, shard started and failed were flipped around by mistake. This commit fixes their naming.

Closes #9440

@clintongormley clintongormley changed the title Internal: Better categorization for transport actions Better categorization for transport actions Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.