Make keepalive pings bidirectional and optimizable #35441

Tim-Brooks · 2018-11-12T04:39:35Z

This is related to #34405 and a follow-up to #34753. It makes a number
of changes to our current keepalive pings.

The ping interval configuration is moved to the ConnectionProfile.
The server channel now responds to pings. This makes the keepalive
pings bidirectional.
On the client-side, the pings can now be optimized away. What this
means is that if the channel has received a message and sent a message
since the last pinging round, the ping is not sent for this round.

elasticmachine · 2018-11-12T04:39:37Z

Pinging @elastic/es-distributed

s1monw

this looks awesome @tbrooks8 I left a bunch of comments.

modules/transport-netty4/src/main/java/org/elasticsearch/transport/netty4/Netty4TcpChannel.java

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

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

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

s1monw · 2018-11-13T09:47:16Z

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

+        }
+    }
+
+    interface PingSender {


Just an idea, stuff like this comes up all the time. Can we extract a AsyncBiFunction and AsyncFunction that is equivalent to Function and BiFunction then we can cover these cases in the future. In any case we should mark this as a FunctionalInterface?

send is a void method that takes three arguments. It would be TriConsumer. Is that something you still want? I did add the FunctionalInterface.

hmm this is what I had in mind:

interface AsyncBiFunction<A,B,C> { void run(A a, B b, ActionListener<C>); }

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

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

s1monw

looks good, left 2 comments

s1monw · 2018-11-15T10:37:23Z

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

+        private volatile long lastReadTime;
+        private volatile long lastWriteTime;
+
+        public ChannelStats() {


I wonder if it makes sense to have a LongSupplier passed to it then you don't need to pass the value to the mark methods?

This feels similar to the issue with described in (#35441 (comment)). If ChannelStats must have a LongSupplier, then every different implementation must have ThreadPool passed around everywhere. Which I would kind of like to avoid doing.

well it must have some kind of LongSupplier that's the point. The threadpool can be one impl of it.

s1monw · 2018-11-15T10:42:26Z

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

+
+        public ChannelStats() {
+            // We set the initial value to a day in the past to ensure that the next read or write time is
+            // greater than the initial time. This is important because we mark reads and writes with our


why does it need to be higher? the comparison here https://github.com/elastic/elasticsearch/pull/35441/files#diff-2dc062611a89dbcea48ead749790ac07R205 is <= 0 so it's fine to be equal? not sure I follow

The cached time updater ticks every 200 ms. So System.nano() can return a time that is in the future of the time it returns.

I don't understand. We always get the time from the threadpool so how can it be in the future?

In the version you reviewed here, we still used System.nanoTime() when the ChannelStats were ctored. That was to avoid passing the thread pool to each channel impl (even if it was a method ref for LongSupplier). And then passing that to the ChannelStats.

Since ChannelStats are now ctored in TcpTransport, it is very easy to pass the thread pool in.

Tim-Brooks · 2018-11-15T17:42:17Z

@s1monw - I added an AsyncBiConsumer. That naming seemed most consistent with java.util classes.

In regards to the ChannelStats - I could move them off of the channel and into a ConcurrentMap channel -> channel stats in TcpTransport. That avoids the need for all the different Transport implementations to handle creating them, etc. Let me know what you think.

Tim-Brooks · 2018-11-15T17:48:34Z

To add to this, we already have a map that is pretending to be a set for accepted channels in TcpTransport:

private final Set<TcpChannel> acceptedChannels = Collections.newSetFromMap(new ConcurrentHashMap<>());

I could add one for outbound client channels also. And have them be maps of channels -> channel stats. And then the underlying transport implementations to not need to know anything about channel stats.

s1monw · 2018-11-19T09:50:11Z

I added an AsyncBiConsumer. That naming seemed most consistent with java.util classes.

I disagree, a consumer doesn't produce anything. We have a function that passes it return value to a listener. I think that is more intuitive and correct

n regards to the ChannelStats - I could move them off of the channel and into a ConcurrentMap channel -> channel stats in TcpTransport. That avoids the need for all the different Transport implementations to handle creating them, etc. Let me know what you think.

+1

I could add one for outbound client channels also. And have them be maps of channels -> channel stats. And then the underlying transport implementations to not need to know anything about channel stats.

+1

this can also be a followup IMO

Tim-Brooks · 2018-11-19T21:46:13Z

@s1monw I've updated with changes.

s1monw

@tbrooks8 I'm afraid I'm not a huge fan of these maps. I didn't realize that is what you meant, I think we should not do an additional lookup for every send / receive. I do think we can maybe go back to what we had before and simplify it. Instead of having a differentiator between read and write can be have long TcpChannel#getLastAccessTime() and void TcpChannel#markAccessed(long time) This would keep the interface simple and we don't have to pass around long supplier etc. WDYT?

Tim-Brooks · 2018-11-28T01:56:04Z

@s1monw I've updated with changes.

Instead of having a differentiator between read and write can be have long TcpChannel#getLastAccessTime() and void TcpChannel#markAccessed(long time)

Sure okay.

we don't have to pass around long supplier etc. WDYT?

I mean the main problem is that, since we are using a relative clock, we must initialize the field to some value. The problem with prior PR iterations is that if we do not have the LongSupplier in the ctor, we must initialize with System#nanotime which is not guaranteed to be monotonic when compared to ThreadPool#relativeTimeInMillis. Anyway, I resolved that by marking that the channel has been accessed immediately after it has been created. That happens prior to it being scheduled for keepalives.

s1monw

LGTM

This is related to elastic#34405 and a follow-up to elastic#34753. It makes a number of changes to our current keepalive pings. The ping interval configuration is moved to the ConnectionProfile. The server channel now responds to pings. This makes the keepalive pings bidirectional. On the client-side, the pings can now be optimized away. What this means is that if the channel has received a message or sent a message since the last pinging round, the ping is not sent for this round.

This is related to #34405 and a follow-up to #34753. It makes a number of changes to our current keepalive pings. The ping interval configuration is moved to the ConnectionProfile. The server channel now responds to pings. This makes the keepalive pings bidirectional. On the client-side, the pings can now be optimized away. What this means is that if the channel has received a message or sent a message since the last pinging round, the ping is not sent for this round.

Prior to elastic#35441 `ConnectionManager` had a `Lifecycle` object to support the ping runnable. After that commit, the connection amanger only needs the existing `AtomicBoolean` to indicate if it is running.

Prior to #35441 `ConnectionManager` had a `Lifecycle` object to support the ping runnable. After that commit, the connection amanger only needs the existing `AtomicBoolean` to indicate if it is running.

Tim-Brooks added 12 commits November 8, 2018 15:38

WIP

3dc8eb1

WIP

8a5f887

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

f60d26a

WIP

29cd642

WIP

fe707d8

WIP

67ec991

WIP

a234ae1

Finish unit test

d2e43ee

WIP

2ae773a

WIP

a3cefcc

WIP

57f371e

WIP

ead5531

Tim-Brooks added >non-issue :Distributed Coordination/Network Http and internode communication implementations v7.0.0 v6.6.0 labels Nov 12, 2018

Tim-Brooks requested review from javanna, s1monw and original-brownbear November 12, 2018 04:39

Tim-Brooks added 7 commits November 11, 2018 23:21

Fix issues

3196aae

Fix tests

1761b75

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

df93fc5

Close keep alive

63c65a4

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

b66f81a

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

5e3a156

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

8bab5a9

s1monw suggested changes Nov 13, 2018

View reviewed changes

Changes from review

8564eee

Tim-Brooks requested a review from s1monw November 13, 2018 17:42

s1monw suggested changes Nov 15, 2018

View reviewed changes

Tim-Brooks added 2 commits November 15, 2018 09:58

Add async bi consumer

d7e920f

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

55ba446

Tim-Brooks requested a review from s1monw November 15, 2018 17:42

Tim-Brooks added 3 commits November 19, 2018 11:21

Changes from review

3320bbe

checkstyle

ac7cdae

Remove assertion

135f74a

Tim-Brooks added 2 commits November 19, 2018 18:21

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

8a250c6

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

cdd1ec3

s1monw reviewed Nov 20, 2018

View reviewed changes

javanna mentioned this pull request Nov 23, 2018

Improve CCS network faults detection #34405

Closed

5 tasks

Tim-Brooks added 2 commits November 27, 2018 18:06

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

01b74dc

Changes from review

11a7167

Tim-Brooks requested a review from s1monw November 28, 2018 13:52

s1monw approved these changes Nov 29, 2018

View reviewed changes

Tim-Brooks merged commit c305f9d into elastic:master Nov 29, 2018

Tim-Brooks mentioned this pull request Nov 29, 2018

Remove Lifecycle from ConnectionManager #36092

Merged

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

Tim-Brooks deleted the keep_alive_changes branch December 18, 2019 14:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make keepalive pings bidirectional and optimizable #35441

Make keepalive pings bidirectional and optimizable #35441

Tim-Brooks commented Nov 12, 2018

elasticmachine commented Nov 12, 2018

s1monw left a comment

s1monw Nov 13, 2018

Tim-Brooks Nov 13, 2018

s1monw Nov 15, 2018

s1monw left a comment

s1monw Nov 15, 2018

Tim-Brooks Nov 15, 2018

s1monw Nov 19, 2018

s1monw Nov 15, 2018

Tim-Brooks Nov 15, 2018

s1monw Nov 19, 2018

Tim-Brooks Nov 19, 2018

Tim-Brooks commented Nov 15, 2018 •

edited

Loading

Tim-Brooks commented Nov 15, 2018

s1monw commented Nov 19, 2018

Tim-Brooks commented Nov 19, 2018

s1monw left a comment

Tim-Brooks commented Nov 28, 2018

s1monw left a comment

Make keepalive pings bidirectional and optimizable #35441

Make keepalive pings bidirectional and optimizable #35441

Conversation

Tim-Brooks commented Nov 12, 2018

elasticmachine commented Nov 12, 2018

s1monw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1monw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tim-Brooks commented Nov 15, 2018 • edited Loading

Tim-Brooks commented Nov 15, 2018

s1monw commented Nov 19, 2018

Tim-Brooks commented Nov 19, 2018

s1monw left a comment

Choose a reason for hiding this comment

Tim-Brooks commented Nov 28, 2018

s1monw left a comment

Choose a reason for hiding this comment

Tim-Brooks commented Nov 15, 2018 •

edited

Loading