Elasticsearch: Rename TransportXXXAction classes to XXXTransportAction
Just a proposal here. Feel free to close if non sense.
We have today the following classes for actions:
XXXActionXXXRequestXXXRequestBuilderXXXResponseTransportXXXAction
When you try to reduce the number of packages, let say for a plugin for example, you might want to put all your actions within the same package. For example, see the reindex plugin:
It has (skipping non related classes):
├── DeleteByQueryAction.java
├── DeleteByQueryRequest.java
├── DeleteByQueryRequestBuilder.java
├── ReindexAction.java
├── ReindexRequest.java
├── ReindexRequestBuilder.java
├── RestDeleteByQueryAction.java
├── RestReindexAction.java
├── RestRethrottleAction.java
├── RestUpdateByQueryAction.java
├── RethrottleAction.java
├── RethrottleRequest.java
├── RethrottleRequestBuilder.java
├── TransportDeleteByQueryAction.java
├── TransportReindexAction.java
├── TransportRethrottleAction.java
├── TransportUpdateByQueryAction.java
├── UpdateByQueryAction.java
├── UpdateByQueryRequest.java
├── UpdateByQueryRequestBuilder.java
As a developer, I have to scroll up and down every time when I want to open the TransportAction associated with a given Action.
The proposal is to rename TransportXXXAction to XXXTransportAction so all those related classes will be grouped all together.
Thoughts?
Depending on the result of this discussion, we can also imagine a follow up for RestXXXAction which could become XXXRestAction.
The previous example would become:
├── DeleteByQueryAction.java
├── DeleteByQueryRequest.java
├── DeleteByQueryRequestBuilder.java
├── DeleteByQueryRestAction.java
├── DeleteByQueryTransportAction.java
├── ReindexAction.java
├── ReindexRequest.java
├── ReindexRequestBuilder.java
├── ReindexRestAction.java
├── ReindexTransportAction.java
├── RethrottleAction.java
├── RethrottleRequest.java
├── RethrottleRequestBuilder.java
├── RethrottleRestAction.java
├── RethrottleTransportAction.java
├── UpdateByQueryAction.java
├── UpdateByQueryRequest.java
├── UpdateByQueryRequestBuilder.java
├── UpdateByQueryRestAction.java
├── UpdateByQueryTransportAction.java
dadoonet
All 11 comments
+1 on doing both. It'd make reading reindex easier and it only has four
actions.
On Aug 2, 2016 5:48 AM, "David Pilato" [email protected] wrote:
Just a proposal here. Feel free to close if non sense.
We have today the following classes for actions:
- XXXAction
- XXXRequest
- XXXRequestBuilder
- XXXResponse
- TransportXXXAction
When you try to reduce the number of packages, let say for a plugin for
example, you might want to put all your actions within the same package.
For example, see the reindex plugin
https://github.com/elastic/elasticsearch/tree/master/modules/reindex/src/main/java/org/elasticsearch/index/reindex
:It has (skipping non related classes):
├── DeleteByQueryAction.java
├── DeleteByQueryRequest.java
├── DeleteByQueryRequestBuilder.java
├── ReindexAction.java
├── ReindexRequest.java
├── ReindexRequestBuilder.java
├── RestDeleteByQueryAction.java
├── RestReindexAction.java
├── RestRethrottleAction.java
├── RestUpdateByQueryAction.java
├── RethrottleAction.java
├── RethrottleRequest.java
├── RethrottleRequestBuilder.java
├── TransportDeleteByQueryAction.java
├── TransportReindexAction.java
├── TransportRethrottleAction.java
├── TransportUpdateByQueryAction.java
├── UpdateByQueryAction.java
├── UpdateByQueryRequest.java
├── UpdateByQueryRequestBuilder.javaAs a developer, I have to scroll up and down every time when I want to
open the TransportAction associated with a given Action.The proposal is to rename TransportXXXAction to XXXTransportAction so all
those related classes will be grouped all together.Thoughts?
Depending on the result of this discussion, we can also imagine a follow
up for RestXXXAction which could become XXXRestAction.
The previous example would become:├── DeleteByQueryAction.java
├── DeleteByQueryRequest.java
├── DeleteByQueryRequestBuilder.java
├── DeleteByQueryRestAction.java
├── DeleteByQueryTransportAction.java
├── ReindexAction.java
├── ReindexRequest.java
├── ReindexRequestBuilder.java
├── ReindexRestAction.java
├── ReindexTransportAction.java
├── RethrottleAction.java
├── RethrottleRequest.java
├── RethrottleRequestBuilder.java
├── RethrottleRestAction.java
├── RethrottleTransportAction.java
├── UpdateByQueryAction.java
├── UpdateByQueryRequest.java
├── UpdateByQueryRequestBuilder.java
├── UpdateByQueryRestAction.java
├── UpdateByQueryTransportAction.java—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/elastic/elasticsearch/issues/19737, or mute the
thread
https://github.com/notifications/unsubscribe-auth/AANLogFbAgj3pe2X-VUY-76QCk74r6QCks5qbxJqgaJpZM4JabZN
.
nik9000
on 2 Aug 2016
+1
abeyad
on 2 Aug 2016
Conceptually, the class name TransportReindexAction communicates _exactly_ what the class is doing, it transports the reindex action. However, the name ReindexTransportAction does not communicate this. For this reason, I prefer the current naming scheme and I'm opposed to this change.
jasontedor
on 2 Aug 2016
it transports the reindex action
I think of it as _implementing_ the reindex action rather than _transporting_ it.
nik9000
on 2 Aug 2016
I think of it as implementing the reindex action rather than transporting it.
That was my initial thought as well. If it is, we should may be rename with something more obvious like ReindexActionImplementation (just a proposal here)?
dadoonet
on 2 Aug 2016
I think of it as implementing the reindex action rather than transporting it.
But that's what I mean? It coordinates and executes the entire action. Look at something like TransportBroadcastByNodeAction. At the start of the request, it transports a node-level request to each node responsible for executing node-level operations. Each node receives these requests, executes the operations, and transports the result back to the coordinating node. The coordinating node then collects these results, and notifies the action listener.
jasontedor
on 2 Aug 2016
It coordinates and executes the entire action.
I've always thought of the coordination and execution portions as "more than" transporting the action though.
nik9000
on 2 Aug 2016
I've always referred to these classes in my head as the "transport actions" as opposed to the "rest actions". The latter is the code executed on the rest layer which then delegates to the former, which is the corresponding action that executes on the transport layer. That's why I don't read "transport" as a verb and I would be in favour of the proposed rename.
And the fact that it would help keeping classes for the same api closer rather than far apart is a plus too.
javanna
on 2 Aug 2016
Do we have any veto on this?
It's "just" about renaming. Should I try to get that before 5.0.0 Beta or GA? Or do we prefer doing this for 6.0?
@clintongormley @s1monw what do you think?
dadoonet
on 5 Sep 2016
Should I close it or work on a PR against 6.0 branch?
dadoonet
on 9 Mar 2017
Given that we didn't reach consensus on this, I don't think we should do it. Closing.
javanna
on 5 May 2017
Related issues
jpountz
·
3Comments
martijnvg
·
3Comments
dadoonet
·
3Comments
abtpst
·
3Comments
Praveen82
·
3Comments
Most helpful comment
I've always referred to these classes in my head as the "transport actions" as opposed to the "rest actions". The latter is the code executed on the rest layer which then delegates to the former, which is the corresponding action that executes on the transport layer. That's why I don't read "transport" as a verb and I would be in favour of the proposed rename.
And the fact that it would help keeping classes for the same api closer rather than far apart is a plus too.