You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2021/09/01 04:00:26 UTC
[GitHub] [iotdb] chengjianyun opened a new issue #3881: [Cluster-refactor] About refine classes name
chengjianyun opened a new issue #3881:
URL: https://github.com/apache/iotdb/issues/3881
> Background: we're working together with @jixuan1989 to refactor cluster module. The dev branch is `cluster-`
After reviewing the code, I think we can divide the current process flow into 3 layers as show in the image and refine the class names in **Transport layer** and **RPC interface layer** to make them clearer.
![Uploading Screenshot-2021-09-01-111500.jpg…]()
### Cluster Instance
`ClusterIoTDB` is the server instance which control the sub servers lifecycle.
### Transport Layer
I call this layer as transport layer because the instances in the layer take charge of setup port listening, setup TCP work threads, establish connection, hand over requests... via thrift APIs. They mainly the call thrift initial APIs.
Right now the class in this layer are called `***Service` which I don't think is clear. An idea is rename the classes as `***SubServer`.
### RPC interface Layer
The classes in this layer are the implementation of thrift RPC interfaces which handle the RPC call from thrift client.
Right now the class in this layer are called `***Service`. If we can change the classes name in the `transport port` layer, I think `***Service` here is acceptable. A suggestion is `***RPCHandler` which is better than `***Service` in current implementation IMO. `***Service` could be used to hide the concrete RPC framework like thrift but right now we don't have that layer.
### Executor Layer
The classes in the layer encapsulate the concrete execution logic. Won' analysis here.
## Why
When I first looked at the codes of IoTDB, I lost in the kinds of `***Services`, I want to make it clear to let more dev can hand on codes a bit easier by the opportunity.
I don't align the suggestion changes with the class names in the server module because I don't think sever module is clear enough according to my understanding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] chengjianyun edited a comment on issue #3881: [Cluster-refactor] About refine classes name
Posted by GitBox <gi...@apache.org>.
chengjianyun edited a comment on issue #3881:
URL: https://github.com/apache/iotdb/issues/3881#issuecomment-912364943
> Hi, I am almost free now and can find some time back to the development.
>
> 1. Agree to use `*Server`.
> 2. Agree to move the Thrift interface implementation to `xx.thrift.impl`, either using `*Impl` or `*Service` is fine, but considering the server module, I think `*Impl` is more moderate.
> 3. I hope to rename `MetaGroupMember` to `MetaGroupEngine` and `DataGroupMember` to `DataGroupEngine`.
Welcome back, lol! As I'm working on the refactor client pool part(code compile passed, working on UT fix, latest progress see https://github.com/apache/iotdb/pull/3886), anyone who can help rename the class would be appreciate. But we'd better let the change happen when the refactor almost done to make conflict resolve easier for other contributors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] ericpai commented on issue #3881: [Cluster-refactor] About refine classes name
Posted by GitBox <gi...@apache.org>.
ericpai commented on issue #3881:
URL: https://github.com/apache/iotdb/issues/3881#issuecomment-909960518
Excellent design! When I first scan the current code in cluster module, I can't find the entrance of the server side in each RPC call unless I use the `show implementations` of IDE :( .
Here're my opinions according to your design, only for reference.
1. Transport layer acts as a server indeed, so both `**subServer` and `***Server` are OK. I prefer `***Server` because there's not any other `**Server` classes in naming or concept, in cluster module.
2. The naming of class in **RPC interface layer** should contain `ThriftServerImpl` or `RPCServerImpl`, and place them in a separate package called `xxx.thrift.impl` or `xxx.rpc.impl`. These names clearly indicate that they are implementations of thrift interface. Now they are placed in the `org.apache.iotdb.cluster.server` package.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] ericpai commented on issue #3881: [Cluster-refactor] About refine classes name
Posted by GitBox <gi...@apache.org>.
ericpai commented on issue #3881:
URL: https://github.com/apache/iotdb/issues/3881#issuecomment-909960518
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] cigarl commented on issue #3881: [Cluster-refactor] About refine classes name
Posted by GitBox <gi...@apache.org>.
cigarl commented on issue #3881:
URL: https://github.com/apache/iotdb/issues/3881#issuecomment-909987628
> 2. The naming of class in **RPC interface layer** should contain `ThriftServerImpl` or `RPCServerImpl`, and place them in a separate package called `xxx.thrift.impl` or `xxx.rpc.impl`. These names clearly indicate that they are implementations of thrift interface. Now they are placed in the `org.apache.iotdb.cluster.server` package.
I could't agree more.
Whether it's a service that handles client requests, or a service that communicates internally, it should like a module in server.we should not nest too deeply in the `server`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] jixuan1989 commented on issue #3881: [Cluster-refactor] About refine classes name
Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on issue #3881:
URL: https://github.com/apache/iotdb/issues/3881#issuecomment-912356715
Hi, I am almost free now and can find some time back to the development.
1. Agree to use `*Server`.
2. Agree to move the Thrift interface implementation to `xx.thrift.impl`, either using `*Impl` or `*Service` is fine, but considering the server module, I think `*Impl` is more moderate.
3. I hope to rename `MetaGroupMember` to `MetaGroupEngine` and `DataGroupMember` to `DataGroupEngine`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] ericpai commented on issue #3881: [Cluster-refactor] About refine classes name
Posted by GitBox <gi...@apache.org>.
ericpai commented on issue #3881:
URL: https://github.com/apache/iotdb/issues/3881#issuecomment-909992477
> > 1. The naming of class in **RPC interface layer** should contain `ThriftServerImpl` or `RPCServerImpl`, and place them in a separate package called `xxx.thrift.impl` or `xxx.rpc.impl`. These names clearly indicate that they are implementations of thrift interface. Now they are placed in the `org.apache.iotdb.cluster.server` package.
>
> I could't agree more.
> Whether it's a service that handles client requests, or a service that communicates internally, it should like a module in server.we should not nest too deeply in the `server`.
Yes, their package relationships are more likely siblings, logically. Not parent-children ones.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] github-actions[bot] commented on issue #3881: [Cluster-refactor] About refine classes name
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on issue #3881:
URL: https://github.com/apache/iotdb/issues/3881#issuecomment-909857434
Hi, this is your first issue in IoTDB project. Thanks for your report. Welcome to join the community!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] chengjianyun commented on issue #3881: [Cluster-refactor] About refine classes name
Posted by GitBox <gi...@apache.org>.
chengjianyun commented on issue #3881:
URL: https://github.com/apache/iotdb/issues/3881#issuecomment-912364943
> Hi, I am almost free now and can find some time back to the development.
>
> 1. Agree to use `*Server`.
> 2. Agree to move the Thrift interface implementation to `xx.thrift.impl`, either using `*Impl` or `*Service` is fine, but considering the server module, I think `*Impl` is more moderate.
> 3. I hope to rename `MetaGroupMember` to `MetaGroupEngine` and `DataGroupMember` to `DataGroupEngine`.
Welcome back, lol! As I'm working on the refactor client pool part(code compile passed, working on UT fix, latest progress see https://github.com/apache/iotdb/pull/3886), anyone who can help rename the class would be appreciate. But we'd better let the change happens when the refactor almost done to make conflict resolve easier for other contributors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] github-actions[bot] commented on issue #3881: [Cluster-refactor] About refine classes name
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on issue #3881:
URL: https://github.com/apache/iotdb/issues/3881#issuecomment-909857434
Hi, this is your first issue in IoTDB project. Thanks for your report. Welcome to join the community!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] cigarl commented on issue #3881: [Cluster-refactor] About refine classes name
Posted by GitBox <gi...@apache.org>.
cigarl commented on issue #3881:
URL: https://github.com/apache/iotdb/issues/3881#issuecomment-909987628
> 2. The naming of class in **RPC interface layer** should contain `ThriftServerImpl` or `RPCServerImpl`, and place them in a separate package called `xxx.thrift.impl` or `xxx.rpc.impl`. These names clearly indicate that they are implementations of thrift interface. Now they are placed in the `org.apache.iotdb.cluster.server` package.
I could't agree more.
Whether it's a service that handles client requests, or a service that communicates internally, it should like a module in server.we should not nest too deeply in the `server`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org