Remove client connections from TcpTransport by Tim-Brooks · Pull Request #31886 · elastic/elasticsearch

Tim-Brooks · 2018-07-09T00:06:04Z

This is related to #31835. This commit adds a connection manager that
manages client connections to other nodes. This means that the
TcpTransport no longer maintains a map of nodes that it is connected
to.

elasticmachine · 2018-07-09T00:06:05Z

Pinging @elastic/es-core-infra

Tim-Brooks · 2018-07-09T00:07:43Z

Hey @s1monw

This is the result of some work that I've played around with over the last year. I know that you are assigned to the issue, so you might have your own plan. But I had created a number of branches based on this work at one point or another, so I thought I would push it up here. It is pretty rough currently. There are tests that have not been updated (due to changes in how mock transports work). Additionally, the TcpTransport still holds the connection profiles. That could be resolved in this PR or another.

Essentially, we do not need to merge this. I just wanted to push it up here to show my thoughts on that issue. I see the options as:

You like this approach. Let me know and I can continue with the tedious parts (fixing tests, etc).
You want to go a different route / or have already started work on this issue. Same thing, let me know and I can close this. I was justing pushing it up here so we could discuss.

Long term my view would be that you could have different connection managers for different pieces of functionality (normal cluster connections, ccs, other cross cluster stuff, etc). The connection manager could be ToXContent and we could add a route to access and interrogate all the nodes that we are currently connected to (for monitoring or metrics). These connection managers could live in some map in the TransportService. Or they could be in the respective services. TcpTransport continues to be reduced until it is mostly just opening connections, binding connections, etc. I feel like the serialization and dispatching messages should maybe eventually live closer to the TransportService (or another class). I feel like the TransportService is more the applicaiton level stuff, and the Transport is more low-level create connections stuff.

s1monw

I like it. I left some comments. thanks for taking this on.

s1monw · 2018-07-23T13:22:36Z

server/src/main/java/org/elasticsearch/transport/TransportConnectionListener.java

 */
 public interface TransportConnectionListener {

+    interface NodeConnection {


i think these sub-interfaces are unnecessary. lets just stick with the top level one

s1monw · 2018-07-23T13:23:23Z

server/src/main/java/org/elasticsearch/transport/Transport.java

        void sendRequest(long requestId, String action, TransportRequest request, TransportRequestOptions options) throws
            IOException, TransportException;

+        default boolean supportsPing() {


lets just return a boolean from sendPing and remove this method?

s1monw · 2018-07-23T13:25:33Z

server/src/main/java/org/elasticsearch/transport/Transport.java

+            throw new UnsupportedOperationException("Not support by this connection type");
+        }
+
+        default boolean isClosed() {


I don't see this used or implemented, maybe it's not necessary?

s1monw · 2018-07-23T13:29:35Z

server/src/main/java/org/elasticsearch/transport/ConnectionManager.java

+
+import static org.elasticsearch.common.util.concurrent.ConcurrentCollections.newConcurrentMap;
+
+public class ConnectionManager implements Closeable {


can we add some javadocs?

I would also love to see a unittest for this one. that would be awesome!

s1monw · 2018-07-23T13:31:57Z

server/src/main/java/org/elasticsearch/transport/TcpTransport.java

+            out.writeInt(TcpTransport.PING_DATA_SIZE);
+            pingMessage = out.bytes();
+        } catch (IOException e) {
+            throw new IllegalStateException(e.getMessage(), e); // won't happen


let's throw an assertion error instead. I konw it's preexisting.

s1monw · 2018-07-23T14:46:46Z

server/src/test/java/org/elasticsearch/client/transport/FailAndRetryMockTransport.java

-    @Override
-    public Connection openConnection(DiscoveryNode node, ConnectionProfile profile) throws IOException {
-        return getConnection(node);
+//    @Override


this can go?

s1monw · 2018-07-23T14:46:56Z

server/src/test/java/org/elasticsearch/cluster/NodeConnectionsServiceTests.java

-            connectedNodes.remove(node);
-            listener.onNodeDisconnected(node);
-        }
+//        @Override


this can go as well?

s1monw · 2018-07-23T14:47:11Z

server/src/test/java/org/elasticsearch/transport/RemoteClusterConnectionTests.java

-                    public boolean nodeConnected(DiscoveryNode node) {
-                        return node.equals(connectedNode);
-                    }
+//                    @Override


Tim-Brooks · 2018-07-24T00:45:59Z

@s1monw - Alright. I will work on getting this PR production ready. As you will have noticed (from CI and commented out stuff), there is still some work to do on our whole "mocking network" constructs for tests to pass.

s1monw · 2018-07-25T08:13:08Z

@tbrooks8 thanks man, assign me as a reviewer again once you are ready

Tim-Brooks · 2018-08-02T04:10:27Z

@s1monw - I think this is ready for another view. The moving the connection profile out of the TcpTransport will happen in a follow up. As this PR is already pretty big.

s1monw

I left some minors. This is a great step forward. Lets move on! LGTM

s1monw · 2018-08-13T08:33:21Z

server/src/main/java/org/elasticsearch/transport/CloseableConnection.java

+    }
+
+    @Override
+    public void close() {


you should protect from double closing here.

s1monw · 2018-08-13T08:37:09Z

server/src/main/java/org/elasticsearch/transport/Transport.java

+            return false;
+        }
+
+        void addCloseListener(ActionListener<Void> listener);


can you explain in what case the ActinListener is called and what method of it?

Tim-Brooks · 2018-08-13T19:28:31Z

Jenkins, run sample packaging tests

Tim-Brooks · 2018-08-13T23:20:40Z

@s1monw - I merged this. I started working on the back port to get far enough to know that it is going to be a little tricky. It will require the back port of some other PRs, etc.

And I can do that, before I went through the process I wanted to check if this is something we want for 6.x? The issue (#31835) does not have targeted versions.

…e-types * elastic/master: (199 commits) Watcher: Remove unused hipchat render method (elastic#32211) Watcher: Remove extraneous auth classes (elastic#32300) Watcher: migrate PagerDuty v1 events API to v2 API (elastic#32285) [TEST] Select free port for Minio (elastic#32837) MINOR: Remove `IndexTemplateFilter` (elastic#32841) Core: Add java time version of rounding classes (elastic#32641) Aggregations/HL Rest client fix: missing scores (elastic#32774) HLRC: Add Delete License API (elastic#32586) INGEST: Create Index Before Pipeline Execute (elastic#32786) Fix NOOP bulk updates (elastic#32819) Remove client connections from TcpTransport (elastic#31886) Increase logging testRetentionPolicyChangeDuringRecovery AwaitsFix case-functions.sql-spec Mute security-cli tests in FIPS JVM (elastic#32812) SCRIPTING: Support BucketAggScript return null (elastic#32811) Unmute WildFly tests in FIPS JVM (elastic#32814) [TEST] Force a stop to save rollup state before continuing (elastic#32787) [test] disable packaging tests for suse boxes Mute IndicesRequestIT#testBulk [ML][DOCS] Refer to rules feature as custom rules (elastic#32785) ...

…listeners * elastic/master: Watcher: Remove unused hipchat render method (elastic#32211) Watcher: Remove extraneous auth classes (elastic#32300) Watcher: migrate PagerDuty v1 events API to v2 API (elastic#32285) [TEST] Select free port for Minio (elastic#32837) MINOR: Remove `IndexTemplateFilter` (elastic#32841) Core: Add java time version of rounding classes (elastic#32641) Aggregations/HL Rest client fix: missing scores (elastic#32774) HLRC: Add Delete License API (elastic#32586) INGEST: Create Index Before Pipeline Execute (elastic#32786) Fix NOOP bulk updates (elastic#32819) Remove client connections from TcpTransport (elastic#31886) Increase logging testRetentionPolicyChangeDuringRecovery AwaitsFix case-functions.sql-spec Mute security-cli tests in FIPS JVM (elastic#32812) SCRIPTING: Support BucketAggScript return null (elastic#32811) Unmute WildFly tests in FIPS JVM (elastic#32814) [TEST] Force a stop to save rollup state before continuing (elastic#32787) [test] disable packaging tests for suse boxes Mute IndicesRequestIT#testBulk [ML][DOCS] Refer to rules feature as custom rules (elastic#32785)

s1monw · 2018-08-15T14:07:07Z

@tbrooks8 I think we can stick with 7.x for this. WDYT?

This is related to elastic#31835. This commit adds a connection manager that manages client connections to other nodes. This means that the TcpTransport no longer maintains a map of nodes that it is connected to.

This is related to #31835. This commit adds a connection manager that manages client connections to other nodes. This means that the TcpTransport no longer maintains a map of nodes that it is connected to.

This is a followup to #31886. After that commit the TransportConnectionListener had to be propogated to both the Transport and the ConnectionManager. This commit moves that listener to completely live in the ConnectionManager. The request and response related methods are moved to a TransportMessageListener. That listener continues to live in the Transport class.

DaveCTurner · 2018-08-20T11:31:47Z

test/framework/src/main/java/org/elasticsearch/test/transport/CapturingTransport.java

+        StubbableConnectionManager connectionManager = new StubbableConnectionManager(new ConnectionManager(settings, this, threadPool),
+            settings, this, threadPool);
+        connectionManager.setDefaultNodeConnectedBehavior((cm, discoveryNode) -> true);
+        connectionManager.setDefaultConnectBehavior((cm, discoveryNode) -> new Connection() {


Bit late to the party here, but could this have been the following?

connectionManager.setDefaultConnectBehavior((cm, discoveryNode) -> openConnection(discoveryNode, null));

That'd let me continue to override the default connection behaviour by subclassing CapturingTransport.

Yeah I think that would work. Feel free to make that change in a PR where you need to override the method.

Great. I opened #33012.

This is related to #31835. This commit adds a connection manager that manages client connections to other nodes. This means that the TcpTransport no longer maintains a map of nodes that it is connected to.

This is a followup to #31886. After that commit the TransportConnectionListener had to be propogated to both the Transport and the ConnectionManager. This commit moves that listener to completely live in the ConnectionManager. The request and response related methods are moved to a TransportMessageListener. That listener continues to live in the Transport class.

Tim-Brooks added 4 commits July 6, 2018 11:45

Work on connection manager

6b6981f

WIP

744375e

WIP

002b371

At least fix checkstyle

06a0093

Tim-Brooks added >enhancement review :Distributed/Network Http and internode communication implementations v7.0.0 v6.4.0 labels Jul 9, 2018

Tim-Brooks requested a review from s1monw July 9, 2018 00:06

Tim-Brooks added 3 commits July 8, 2018 18:36

Fix test

f9e7080

Add comment

67bc0b2

Merge remote-tracking branch 'upstream/master' into new_cm

31e3acf

s1monw suggested changes Jul 23, 2018

View reviewed changes

Tim-Brooks added 4 commits July 23, 2018 11:32

Merge remote-tracking branch 'upstream/master' into new_cm

17a334a

Remove additional listeners

c3544ad

Work on fixing tests

fc886bf

Remove unused

f699321

Tim-Brooks added 5 commits July 23, 2018 22:12

Fix checkstyle

03acc19

Work on test infra

796a728

Merge remote-tracking branch 'upstream/master' into new_cm

8005de4

Work on tests

43dd3ad

Remove methods

acb743a

Tim-Brooks added 3 commits July 25, 2018 13:32

Merge remote-tracking branch 'upstream/master' into new_cm

2eacbfa

Merge remote-tracking branch 'upstream/master' into new_cm

ca9fe7c

Use new behaviors

aaf2d87

Tim-Brooks added 4 commits August 1, 2018 17:07

Work on tests

617521f

Fix tests

76d429c

java doc

8c26686

Merge remote-tracking branch 'upstream/master' into new_cm

071490a

Tim-Brooks requested review from jasontedor and s1monw August 2, 2018 04:09

Tim-Brooks added 3 commits August 2, 2018 10:07

Merge remote-tracking branch 'upstream/master' into new_cm

44f6e1d

Merge remote-tracking branch 'upstream/master' into new_cm

4c26165

Merge remote-tracking branch 'upstream/master' into new_cm

453b853

s1monw approved these changes Aug 13, 2018

View reviewed changes

Tim-Brooks added 2 commits August 13, 2018 10:00

Changes from review

d18be8c

Changes docs

f259bae

Tim-Brooks merged commit 10fddb6 into elastic:master Aug 13, 2018

Tim-Brooks mentioned this pull request Aug 17, 2018

Move connection listener to ConnectionManager #32956

Merged

DaveCTurner reviewed Aug 20, 2018

View reviewed changes

Tim-Brooks mentioned this pull request Aug 20, 2018

Move connection listener to ConnectionManager #32992

Merged

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Tim-Brooks deleted the new_cm branch December 18, 2019 14:44


		import static org.elasticsearch.common.util.concurrent.ConcurrentCollections.newConcurrentMap;

		public class ConnectionManager implements Closeable {

Conversation

Tim-Brooks commented Jul 9, 2018

Uh oh!

elasticmachine commented Jul 9, 2018

Uh oh!

Tim-Brooks commented Jul 9, 2018

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tim-Brooks commented Jul 24, 2018

Uh oh!

s1monw commented Jul 25, 2018

Uh oh!

Tim-Brooks commented Aug 2, 2018

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tim-Brooks commented Aug 13, 2018

Uh oh!

Tim-Brooks commented Aug 13, 2018

Uh oh!

s1monw commented Aug 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants