Simplify Unicast Zen Ping by bleskes · Pull Request #22277 · elastic/elasticsearch

bleskes · 2016-12-20T09:50:09Z

The UnicastZenPing shows it's age and is the result of many small changes. The current state of affairs is confusing and is hard to reason about. This PR cleans it up (while following the same original intentions). Highlights of the changes are:

Clear 3 round flow - no interleaving of scheduling.
The previous implementation did a best effort attempt to wait for ongoing pings to be sent and completed. The pings were guaranteed to complete because each used the total ping duration as a timeout. This did make it hard to reason about the total ping duration and the flow of the code. All of this is removed now and ping should just complete within the given duration or not be counted (note that it was very handy for testing, but I move the needed sync logic to the test).
Because of (2) the pinging scheduling changed a bit, to give a chance for the last round to complete. We now ping at the beginning, 1/3 and 2/3 of the duration.
To offset for (3) a bit, incoming ping requests are now added to on going ping collections.
UnicastZenPing never establishes full blown connections (but does reuse them if there). Relates to [CI] MinimumMasterNodesIT.testMultipleNodesShutdownNonMasterNodes sporadically fails #22120
Discovery host providers are only used once per pinging round. Closes FileBasedDiscovery causes connections on every ping round #21739
Usage of the ability to open a connection without connecting to a node ( Add infrastructure to manage network connections outside of Transport/TransportService #22194 ) and shorter connection timeouts helps with connections piling up. Closes Thread leak in TribeNode when a cluster is offline #19370
Beefed up testing and sped them up.
removed light profile from production code

s1monw

looks great, much simpler! I left a bunch of comments.

s1monw · 2016-12-20T12:11:49Z

core/src/main/java/org/elasticsearch/discovery/zen/ElectMasterService.java

 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.node.DiscoveryNode;
 import org.elasticsearch.common.component.AbstractComponent;
-import org.elasticsearch.common.inject.Inject;


s1monw · 2016-12-20T12:14:46Z

core/src/main/java/org/elasticsearch/discovery/zen/UnicastZenPing.java

-                () -> UNICAST_NODE_PREFIX + unicastNodeIdGenerator.incrementAndGet() + "#",
+                UNICAST_NODE_PREFIX,
                resolveTimeout);
        } catch (InterruptedException e) {


please restore the interrupt status here?

this is how it was and we do throw an exception, thus processing the interrupt?

s1monw · 2016-12-20T12:15:43Z

core/src/main/java/org/elasticsearch/discovery/zen/UnicastZenPing.java

+        final AbstractRunnable pingSender = new AbstractRunnable() {
+            @Override
+            public void onFailure(Exception e) {
+                if (e instanceof AlreadyClosedException) {


maybe (e instanceof AlreadyClosedException) == false?

yep. This morphed - I used to have a log there but got annoyed with it (just noise).

s1monw · 2016-12-20T12:17:30Z

core/src/main/java/org/elasticsearch/discovery/zen/UnicastZenPing.java

-        public PingCollection pingCollection() {
-            return pingCollection;
+        public List<DiscoveryNode> getSeedNodes() {
+            checkIfClosed();


we call it ensureOpen everywhere can we do the same here?

good one. Will change.

s1monw · 2016-12-20T12:19:06Z

core/src/main/java/org/elasticsearch/discovery/zen/UnicastZenPing.java

+            }
+        }
+
+        public synchronized Connection addConnectionIfNeeded(TransportAddress address, Connection newConnection) {


hmm that looks weird. Can we maybe use a KeyedLock when we open the connections with IP and port or something like this?

yeah, I wanted to have the simplest construct as it was a rare collision. With the latest code I actually think it's impossible (I dedup on addresses and the connection are private to the pinging round). Will remove.

turns out we do need this protection or something similar. I took another approach, which I think you'd like better.

s1monw · 2016-12-20T12:30:59Z

core/src/main/java/org/elasticsearch/discovery/zen/UnicastZenPing.java

-                            }
+
+                if (connection == null) {
+                    logger.trace("[{}] connecting (light) to {}", pingingRound.id(), node);


do we need this trace log here and if so can we fix it to say temporarily or something like this

I adapted the log message

s1monw · 2016-12-20T12:39:01Z

core/src/main/java/org/elasticsearch/discovery/zen/UnicastZenPing.java

-                } finally {
-                    latch.countDown();
+                logger.trace("[{}] received response from {}: {}", pingingRound.id(), node, Arrays.toString(response.pingResponses));
+                if (pingingRound.isClosed() == false) {


just flip it then you don't need to negate

s1monw · 2016-12-20T12:39:18Z

core/src/main/java/org/elasticsearch/discovery/zen/UnicastZenPing.java

-                    }
-                } finally {
-                    latch.countDown();
+                logger.trace("[{}] received response from {}: {}", pingingRound.id(), node, Arrays.toString(response.pingResponses));


if you keep the trace maybe use a logging guard here?

sure thing, will add.

s1monw · 2016-12-20T12:44:44Z

core/src/main/java/org/elasticsearch/transport/ConnectionProfile.java

+     */
+    public static ConnectionProfile getLightProfileWithTimeout(@Nullable TimeValue connectTimeout,
+                                                               @Nullable TimeValue handshakeTimeout) {
+        return new ConnectionProfile(


I wonder if we should do this. I think we should move the LIGHT_PROFILE into tests somewhere and then require every special use to build it's own. The problem I have here is that the getLightProfileWithTimeout shares one connection across all uses. I think in the case of ping we should only use 1 connection for PING and 0 for the others. that will cause an exception if it's used in a wrong context. makes sense?

I tried to implement your suggestion and I think it looks good. will push shortly.

s1monw · 2016-12-20T12:44:50Z

core/src/main/java/org/elasticsearch/transport/TransportService.java

-     * @return the connected node
-     * @throws ConnectTransportException if the connection failed
-     * @throws IllegalStateException if the handshake failed
-     */


bleskes · 2016-12-20T19:44:17Z

@s1monw I pushed more commits addressing your feedback. Let me know what you think.

jasontedor · 2016-12-20T20:37:17Z

core/src/test/java/org/elasticsearch/discovery/zen/UnicastZenPingTests.java

 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;

+@TestLogging("org.elasticsearch.transport:TRACE,org.elasticsearch.discovery.zen:TRACE")


This logging was initially added to just testSimplePings to chase a race. The race has not reproduced since adding this logging. I think that we should drop the logging and and then address if the race comes back since you've changed how these things are handled.

s1monw

left some minors LGTM otherwise

s1monw · 2016-12-21T07:00:52Z

core/src/main/java/org/elasticsearch/discovery/zen/UnicastZenPing.java

+        public Connection getOrConnect(DiscoveryNode node) throws IOException {
+            Connection result;
+            try (Releasable ignore = connectionLock.acquire(node.getAddress())) {
+                result = tempConnections.get(node.getAddress());


maybe use computeIfAbsent()?

the problem is the IOException that can be thrown while making a connection.

s1monw · 2016-12-21T07:01:58Z

core/src/main/java/org/elasticsearch/discovery/zen/UnicastZenPing.java

        final Map<TransportAddress, DiscoveryNode> uniqueNodesByAddress =
            Stream.concat(pingingRound.getSeedNodes().stream(), nodesFromResponses.stream())
-                .collect(Collectors.toMap(DiscoveryNode::getAddress, n -> n, (n1, n2) -> n1));
+                .collect(Collectors.toMap(DiscoveryNode::getAddress, node -> node, (n1, n2) -> n1));


you didn't like Function.identity() ?

I did but I used it wrong (as a function reference). Using it right works of course .. zzzz

s1monw · 2016-12-21T07:02:40Z

core/src/main/java/org/elasticsearch/discovery/zen/UnicastZenPing.java

+                    }
                } else {
-                    logger.trace("[{}] skipping received response from {}. already closed", pingingRound.id(), node);
+                    Arrays.asList(response.pingResponses).forEach(pingingRound::addPingResponseToCollection);


Arrays.asStream(response.pingResponses) would not materialize it

bleskes · 2016-12-21T14:10:20Z

thx @s1monw. I'll wait a day before backporting

* master: Simplify Unicast Zen Ping (elastic#22277) Replace IndicesQueriesRegistry (elastic#22289) Fixed document mistake and fit for 5.1.1 API [TEST] improve error message in ESTestCase#assertWarnings [TEST] remove deleted test classes from checkstyle suppressions [TEST] make ESSingleNodeTestCase tests repeatable (elastic#22283) Link for setting page in elasticsearch.yml is outdated Factor out sort values from InternalSearchHit (elastic#22080) Add ID for percolate query to Java API docs x_refresh.yaml tests should use unique index names and doc ids to ease debugging IndicesStoreIntegrationIT should not use start recovery sending as an indication that the recovery started Added base class for testing aggregators and some initial tests for `terms`, `top_hits` and `min` aggregations. Add link to foreach processor to ingest-attachment.asciidoc

…r being closed This may cause them to leak. Provisioning for it was made in #22277 but sadly a crucial ensureOpen call was forgotten

@OverRide

…otification task Not doing this made it difficult to establish a happens before relationship between connecting to a node and adding a listeners. Causing test code like this to fail sproadically: ``` // connection to reuse handleA.transportService.connectToNode(handleB.node); // install a listener to check that no new connections are made handleA.transportService.addConnectionListener(new TransportConnectionListener() { @OverRide public void onConnectionOpened(DiscoveryNode node) { fail("should not open any connections. got [" + node + "]"); } }); ``` relates to #22277

The `UnicastZenPing` shows it's age and is the result of many small changes. The current state of affairs is confusing and is hard to reason about. This PR cleans it up (while following the same original intentions). Highlights of the changes are: 1) Clear 3 round flow - no interleaving of scheduling. 2) The previous implementation did a best effort attempt to wait for ongoing pings to be sent and completed. The pings were guaranteed to complete because each used the total ping duration as a timeout. This did make it hard to reason about the total ping duration and the flow of the code. All of this is removed now and ping should just complete within the given duration or not be counted (note that it was very handy for testing, but I move the needed sync logic to the test). 3) Because of (2) the pinging scheduling changed a bit, to give a chance for the last round to complete. We now ping at the beginning, 1/3 and 2/3 of the duration. 4) To offset for (3) a bit, incoming ping requests are now added to on going ping collections. 5) UnicastZenPing never establishes full blown connections (but does reuse them if there). Relates to #22120 6) Discovery host providers are only used once per pinging round. Closes #21739 7) Usage of the ability to open a connection without connecting to a node ( #22194 ) and shorter connection timeouts helps with connections piling up. Closes #19370 8) Beefed up testing and sped them up. 9) removed light profile from production code

…r being closed This may cause them to leak. Provisioning for it was made in #22277 but sadly a crucial ensureOpen call was forgotten

@OverRide

…otification task Not doing this made it difficult to establish a happens before relationship between connecting to a node and adding a listeners. Causing test code like this to fail sproadically: ``` // connection to reuse handleA.transportService.connectToNode(handleB.node); // install a listener to check that no new connections are made handleA.transportService.addConnectionListener(new TransportConnectionListener() { @OverRide public void onConnectionOpened(DiscoveryNode node) { fail("should not open any connections. got [" + node + "]"); } }); ``` relates to #22277

bleskes · 2016-12-23T13:17:15Z

this is now backported to 5.x as well.

bleskes added 9 commits December 18, 2016 23:11

initial implementation

2a8b4b9

speed up pinging tests

5c9cf3d

linting

843dfad

fix FileBasedUnicastHostsProviderTests

29a3701

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

05c08c2

fix racing conditions in waiting for completeness

ccaf3d1

better dedupping en using non-async and exact counters

7a88183

add a test for remembering incoming pings

c7a1b48

use light connections with the right timeout

4e0eeb2

bleskes added :Distributed/Discovery-Plugins Anything related to our integration plugins with EC2, GCP and Azure >enhancement v5.2.0 v6.0.0-alpha1 labels Dec 20, 2016

s1monw suggested changes Dec 20, 2016

View reviewed changes

bleskes added 5 commits December 20, 2016 16:14

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

20351b1

feedback

8e4ae58

feedback

ad08e2a

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

6ba86a0

remove LIGHT_PROFILE in favor of dedicated single channel profiles

3939228

TransportClientNodesService is not ready yet for a single channel type

fec0826

jasontedor reviewed Dec 20, 2016

View reviewed changes

remove trace logging

58ce692

s1monw approved these changes Dec 21, 2016

View reviewed changes

feedback

8959ae6

bleskes force-pushed the unicast_zen_cleanup branch from 43f8287 to 8959ae6 Compare December 21, 2016 08:07

bleskes merged commit 0e9186e into elastic:master Dec 21, 2016

bleskes deleted the unicast_zen_cleanup branch December 21, 2016 14:10

jasontedor mentioned this pull request Dec 21, 2016

avoid repeat connections in pings every round #21812

Closed

bleskes added a commit that referenced this pull request Dec 22, 2016

UnicastZenPing's PingingRound should prevent opening connections afte…

13c5881

…r being closed This may cause them to leak. Provisioning for it was made in #22277 but sadly a crucial ensureOpen call was forgotten

bleskes added a commit that referenced this pull request Dec 23, 2016

UnicastZenPing's PingingRound should prevent opening connections afte…

be74fc2

…r being closed This may cause them to leak. Provisioning for it was made in #22277 but sadly a crucial ensureOpen call was forgotten

ywelsch mentioned this pull request Dec 28, 2016

Properly configure Netty 3 ClientBootstrap when using custom connection profile #22363

Merged

tlrx mentioned this pull request Jan 5, 2017

Avoid zen pinging threads to pile up #19719

Closed

Conversation

bleskes commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

bleskes commented Dec 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

bleskes commented Dec 21, 2016

Uh oh!

bleskes commented Dec 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bleskes commented Dec 20, 2016 •

edited

Loading