[jira] [Updated] (IGNITE-13098) TcpCommunicationSpi split to independent classes

["Stepachev Maksim (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/IGNITE-13098?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Stepachev Maksim updated IGNITE-13098:
--------------------------------------
    Description: 
h2. Description

This ticket describes  requirements for TcpCommunicationSpi refactoring. The main goal is to split the class without changing behavior and public API.

*Actual problem:*

CurrentlyTcpCommunicationSpi has over 5K lines and includes about15+ inner classes like:
 # ShmemAcceptWorker
 # SHMemHandshakeClosure
 # ShmemWorker
 # CommunicationDiscoveryEventListener
 # CommunicationWorker
 # ConnectFuture
 # ConnectGateway
 # ConnectionKey
 # ConnectionPolicy
 # DisconnectedSessionInfo
 # FirstConnectionPolicy
 # HandshakeTimeoutObject
 # RoundRobinConnectionPolicy
 # TcpCommunicationConnectionCheckFuture
 # TcpCommunicationSpiMBeanImpl

In addition, it contains logic of client connection life cycle, nio server handler, and handshake handler.

The classes above have cyclic dependencies and high coupling.The whole mechanism works because classes have access to each other via parent class references. As a result, initialization of class isn't consistent. By consistent I mean that class created via constructor is ready to be used. All of the classes work with context and shareproperties everywhere.

Many methods of TcpCommunicationSpi don’t have a single responsibility. Example is getNodeAttribute:,it makes client reservation,  takes the IP address of the node and provides attributes.

It works fine and we usually don’t have reasons to change anything. But if you want to create a test that has a little different behavior than a blocking message, you can't mock or change the behavior of inner classes. For example, test covering change in the handshake process. Some people make test methods in public API like "closeConnections" or "openSocketChannel" because the current design isn't fine for it. It also takes a lot of time for test development for minor changes.

*Solution:*

The scope of work is big and communication spi is place which should be changed carefully. I recommend to make this refactoring step by step.
 * The first idea is to split the parent class into independent classes and move them to the internal package. We should achieveSOLID when it’s done.
 * Extract spread logic to appropriate classes like ClientPool, HandshakeHandler, etc.
 * Make a common transfer object for TCSpi configuration.
 * Make dependencies direct if it is possible.
 * Initialize all dependencies in one place.
 * Make child classes context-free.
 * Try to do classes more testable.
 * Use the idea of dependency injection without a framework for it.

*Benefits:*

With the ability to write truly jUnit-style tests and cover functionality with better testing we get a way to easier develop new features and optimizations needed in such low-level components as TcpCommunicationSpi.

Examples of features that improve usability of Apache Ignite a lot are: inverse communication connection with optimizations and connection multiplexing. Both of the features could be used in environments with restricted network connectivity (e.g. when connections between nodes could be established only in one direction).

  was:TcpCommunicationSpi split to independent classes


> TcpCommunicationSpi split to independent classes
> ------------------------------------------------
>
>                 Key: IGNITE-13098
>                 URL: https://issues.apache.org/jira/browse/IGNITE-13098
>             Project: Ignite
>          Issue Type: Bug
>         Environment: TcpCommunicationSpi split to independent classes
>            Reporter: Stepachev Maksim
>            Assignee: Stepachev Maksim
>            Priority: Major
>
> h2. Description
> This ticket describes  requirements for TcpCommunicationSpi refactoring. The main goal is to split the class without changing behavior and public API.
> *Actual problem:*
> CurrentlyTcpCommunicationSpi has over 5K lines and includes about15+ inner classes like:
>  # ShmemAcceptWorker
>  # SHMemHandshakeClosure
>  # ShmemWorker
>  # CommunicationDiscoveryEventListener
>  # CommunicationWorker
>  # ConnectFuture
>  # ConnectGateway
>  # ConnectionKey
>  # ConnectionPolicy
>  # DisconnectedSessionInfo
>  # FirstConnectionPolicy
>  # HandshakeTimeoutObject
>  # RoundRobinConnectionPolicy
>  # TcpCommunicationConnectionCheckFuture
>  # TcpCommunicationSpiMBeanImpl
> In addition, it contains logic of client connection life cycle, nio server handler, and handshake handler.
> The classes above have cyclic dependencies and high coupling.The whole mechanism works because classes have access to each other via parent class references. As a result, initialization of class isn't consistent. By consistent I mean that class created via constructor is ready to be used. All of the classes work with context and shareproperties everywhere.
> Many methods of TcpCommunicationSpi don’t have a single responsibility. Example is getNodeAttribute:,it makes client reservation,  takes the IP address of the node and provides attributes.
> It works fine and we usually don’t have reasons to change anything. But if you want to create a test that has a little different behavior than a blocking message, you can't mock or change the behavior of inner classes. For example, test covering change in the handshake process. Some people make test methods in public API like "closeConnections" or "openSocketChannel" because the current design isn't fine for it. It also takes a lot of time for test development for minor changes.
> *Solution:*
> The scope of work is big and communication spi is place which should be changed carefully. I recommend to make this refactoring step by step.
>  * The first idea is to split the parent class into independent classes and move them to the internal package. We should achieveSOLID when it’s done.
>  * Extract spread logic to appropriate classes like ClientPool, HandshakeHandler, etc.
>  * Make a common transfer object for TCSpi configuration.
>  * Make dependencies direct if it is possible.
>  * Initialize all dependencies in one place.
>  * Make child classes context-free.
>  * Try to do classes more testable.
>  * Use the idea of dependency injection without a framework for it.
> *Benefits:*
> With the ability to write truly jUnit-style tests and cover functionality with better testing we get a way to easier develop new features and optimizations needed in such low-level components as TcpCommunicationSpi.
> Examples of features that improve usability of Apache Ignite a lot are: inverse communication connection with optimizations and connection multiplexing. Both of the features could be used in environments with restricted network connectivity (e.g. when connections between nodes could be established only in one direction).



--
This message was sent by Atlassian Jira
(v8.3.4#803005)