Remove LocalTransport in favor of MockTcpTransport #20695

s1monw · 2016-09-29T10:44:58Z

This change proposes the removal of all non-tcp transport implementations. The
mock transport can be used by default to run tests instead of local transport that has
roughly the same performance compared to TCP or at least not noticeably slower.

This is a master only change, deprecation notice in 5.x will be committed as a
separate change.

This change proposes the removal of all non-tcp transport implementations. The mock transport can be used by defaule to run tests instead of local transport that has roughly the same performance compared to TCP or at least not noticibely slower. This is a master only change, deprecation notice in 5.x will be committed as a separate change.

nik9000 · 2016-10-04T17:29:39Z

core/src/main/java/org/elasticsearch/cluster/node/DiscoveryNode.java

@@ -217,7 +215,7 @@ public DiscoveryNode(StreamInput in) throws IOException {
        this.ephemeralId = in.readString().intern();
        this.hostName = in.readString().intern();
        this.hostAddress = in.readString().intern();
-        this.address = TransportAddressSerializers.addressFromStream(in);
+        this.address = new TransportAddress(in);


nik9000 · 2016-10-04T17:33:19Z

core/src/main/java/org/elasticsearch/common/transport/TransportAddressSerializers.java

-    private static final Map<Short, Writeable.Reader<TransportAddress>> ADDRESS_REGISTRY;
-
-    static {
-        Map<Short, Writeable.Reader<TransportAddress>> registry = new HashMap<>();


Hurray! Another NamedWriteableRegistry clone removed!

nik9000 · 2016-10-04T17:45:04Z

core/src/test/java/org/elasticsearch/client/transport/TransportClientNodesServiceTests.java

-                    //the original listed transport address is kept rather than the one returned from the liveness api
-                    assertThat(localTransportAddress.id(), startsWith("node"));
+                    assertNotEquals(discoveryNode.getAddress(), iteration.livenessAddress);
+                    assertTrue(iteration.nodeAddresses.contains(discoveryNode.getAddress()));


I'd prefer something like assertThat(iteration.nodeAddresses, hasItemInArray(discoveryNode.getAddress())). The error messages are nicer.

nik9000 · 2016-10-04T17:46:59Z

core/src/test/java/org/elasticsearch/cluster/serialization/ClusterSerializationTests.java

 import static org.hamcrest.Matchers.equalTo;

 /**
 *
 */
 public class ClusterSerializationTests extends ESAllocationTestCase {
+    private static final AtomicInteger port = new AtomicInteger(0);


Is this a leftover?

nik9000 · 2016-10-04T17:47:54Z

core/src/test/java/org/elasticsearch/common/network/NetworkModuleTests.java

-            super(null, null, null, null);
-        }
-    }
+//    static class FakeTransport extends MockTcpTransport {


Does this need to be uncommented and used?

Looks like this should just be removed.

nik9000 · 2016-10-04T17:52:04Z

core/src/test/java/org/elasticsearch/discovery/DiscoveryWithServiceDisruptionsIT.java

@@ -121,7 +121,6 @@
 import static org.hamcrest.Matchers.nullValue;

 @ClusterScope(scope = Scope.TEST, numDataNodes = 0, transportClientRatio = 0)
-@ESIntegTestCase.SuppressLocalMode


Note to self - figure out why this doesn't need the traditional zen discovery setting.

Answer to self - because this uses ClusterDiscoveryConfiguration which already comes with it.

nik9000 · 2016-10-04T17:57:14Z

core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateHistogramOffsetIT.java

@@ -58,11 +54,6 @@ private DateTime date(String date) {
        return DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.parser().parseDateTime(date);
    }

-    @Override
-    protected Collection<Class<? extends Plugin>> nodePlugins() {
-        return Collections.singleton(AssertingLocalTransport.TestPlugin.class);


This seems like a weird place for this plugin!

nik9000 · 2016-10-04T18:09:09Z

test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java

        Settings.Builder networkSettings = Settings.builder();
-        final boolean isNetwork;
-        if (noLocal != null && noNetwork != null) {


I'm glad this is gone!

nik9000 · 2016-10-04T18:10:36Z

test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java

@@ -174,6 +184,14 @@ protected void afterAlways(List<Throwable> errors) throws Throwable {
    });

    /**
+     * Generates a new transport address using {@link TransportAddress#META_ADDRESS} with an incrementing port number.
+     * The port number starts at 0 and is reset before each test suite run.


s/before/after/

nik9000 · 2016-10-04T18:15:18Z

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

-
-    @Override
-    protected void handleParsedResponse(final TransportResponse response, final TransportResponseHandler handler) {
-        ElasticsearchAssertions.assertVersionSerializable(VersionUtils.randomVersionBetween(random, minVersion, maxVersion), response,


Are we ok with dropping this assertion and the one in the method below? Should we add them to MockTransportService?

no it serializes by default since it goes across the network

I thought the value we got out of this was in serializing to a random version, not that it serializes at all.

not sure there is value here to be honest if you insist I can add it back

the original reason I added this was to serialize at all

It has caught some interesting bugs in the past for me. Stuff like "don't mutate the request after you've sent it" and "don't mutate the request after you've received it" and "make sure to add version guards on read as well as write".

s1monw · 2016-10-05T20:30:46Z

@nik9000 I pushed updates can you look again

nik9000 · 2016-10-05T20:34:26Z

core/src/main/java/org/elasticsearch/rest/action/cat/RestTasksAction.java

@@ -144,11 +144,7 @@ private void buildRow(Table table, boolean fullId, boolean detailed, DiscoveryNo
        // Node information. Note that the node may be null because it has left the cluster between when we got this response and now.
        table.addCell(fullId ? nodeId : Strings.substring(nodeId, 0, 4));
        table.addCell(node == null ? "-" : node.getHostAddress());
-        if (node != null && node.getAddress() instanceof TransportAddress) {


nik9000 · 2016-10-05T20:37:00Z

Cool.

I'm happy with it. If you want to get it in now and deal with the question of assertVersionSerializable after that is fine. I could work on a followup to get the random version testing back and we could debate its merits there?

…ll incoming and outgoing requests / responses

s1monw · 2016-10-06T08:16:31Z

@nik9000 I added an AssertingTransportInterceptor so we are getting these assertions everywhere randomly!

… blocks on the InternalTestcluster

nik9000

I'm fine with it once the build passes.

Thanks for adding the assertion back. I wouldn't have asked unless I'd seen it catch stuff no other assertions caught.

s1monw · 2016-10-07T07:46:42Z

test this please

TransportAddress used to be customizable per transport but this has been removed a while ago. Therefore we can remove all usage of this method as well. Relates to #20695

Relates to elastic#20695

With #20695 we removed local transport and there is just TransportAddress now. The UnicastHostsProvider currently returns DiscoveryNode instances, where, during pinging, we're actually only making use of the TransportAddress to establish a first connection to the possible new node. To simplify the interface, we can just return a list of transport addresses instead, which means that it's not necessary anymore to create fake node objects in each plugin just to return the address information.

s1monw added >test Issues or PRs that are addressing/adding tests review WIP :Distributed Coordination/Network Http and internode communication implementations v6.0.0-alpha1 labels Sep 29, 2016

s1monw force-pushed the trash_local_transport branch from 29d2a90 to 8abd72c Compare October 4, 2016 07:51

s1monw added 9 commits October 4, 2016 12:12

fix all unittest

dae0a8b

cleanup docs

30b80a1

remove leftover annotations

902382c

fix more unittest

a30ff23

fix more tests

c4fa358

always use non-alpha version in AbstractSimpleTransportTestCase'

473e7aa

ensure all previously suppressing local mode test have zen disco enabled

4e0352f

don't resolve DNS in Ec2DiscoveryTests

e646596

s1monw force-pushed the trash_local_transport branch from 8abd72c to e646596 Compare October 4, 2016 10:12

s1monw removed the WIP label Oct 4, 2016

s1monw added 2 commits October 4, 2016 15:25

remove LocalTransportAddress and make TransportAddress a final class

939f2fd

use mock TCP in TribeUnitTests

774b7bb

nik9000 reviewed Oct 4, 2016

View reviewed changes

s1monw added 3 commits October 5, 2016 22:20

Merge branch 'master' into trash_local_transport

77537da

remove redundant casts

eb18272

apply review comments

e234cbb

nik9000 reviewed Oct 5, 2016

View reviewed changes

s1monw added 2 commits October 6, 2016 09:57

add a poorMansDNS to Ec2DiscoveryTests

6d5dbf4

Add transport interceptor that applies assertVersionSerializable to a…

bbe80ad

…ll incoming and outgoing requests / responses

unfinalize TransportAddress :(

639b6fd

s1monw added 4 commits October 6, 2016 10:38

make TransportAddress final again

ed7fa84

Merge branch 'master' into trash_local_transport

a05eef7

don't use the single argument version of assertVersionSerializable it…

3b48c0c

… blocks on the InternalTestcluster

remove stale javadoc

e5ff2b4

nik9000 approved these changes Oct 6, 2016

View reviewed changes

Merge branch 'master' into trash_local_transport

34e6970

s1monw merged commit 194a6b1 into elastic:master Oct 7, 2016

s1monw added >breaking >breaking-java labels Oct 7, 2016

jasontedor mentioned this pull request Dec 9, 2016

5.1 bootstrap error when setting node.local #22064

Closed

ywelsch added a commit to ywelsch/elasticsearch that referenced this pull request Jun 19, 2018

Return transport addresses from UnicastHostsProvider

e647fa5

Relates to elastic#20695

ywelsch mentioned this pull request Jun 19, 2018

Return transport addresses from UnicastHostsProvider #31426

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove LocalTransport in favor of MockTcpTransport #20695

Remove LocalTransport in favor of MockTcpTransport #20695

s1monw commented Sep 29, 2016

nik9000 Oct 4, 2016

nik9000 Oct 4, 2016

nik9000 Oct 4, 2016

nik9000 Oct 4, 2016

nik9000 Oct 4, 2016

nik9000 Oct 4, 2016

nik9000 Oct 4, 2016

nik9000 Oct 4, 2016

nik9000 Oct 4, 2016

nik9000 Oct 4, 2016

nik9000 Oct 4, 2016

nik9000 Oct 4, 2016

s1monw Oct 5, 2016

nik9000 Oct 5, 2016

s1monw Oct 5, 2016

s1monw Oct 5, 2016

nik9000 Oct 5, 2016

s1monw commented Oct 5, 2016

nik9000 Oct 5, 2016

nik9000 commented Oct 5, 2016

s1monw commented Oct 6, 2016

nik9000 left a comment

s1monw commented Oct 7, 2016

Remove LocalTransport in favor of MockTcpTransport #20695

Remove LocalTransport in favor of MockTcpTransport #20695

Conversation

s1monw commented Sep 29, 2016

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

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

Choose a reason for hiding this comment

s1monw commented Oct 5, 2016

Choose a reason for hiding this comment

nik9000 commented Oct 5, 2016

s1monw commented Oct 6, 2016

nik9000 left a comment

Choose a reason for hiding this comment

s1monw commented Oct 7, 2016