From 693e14a14d350dbabcbd88ef999123a6d7f2d27a Mon Sep 17 00:00:00 2001 From: Charles Allen Date: Thu, 8 Oct 2015 09:53:05 -0700 Subject: [PATCH 1/3] Add unit tests for ipv6 in ServerDiscoverySelectorTest --- .../ServerDiscoverySelectorTest.java | 57 +++++++++++++++++-- 1 file changed, 53 insertions(+), 4 deletions(-) diff --git a/server/src/test/java/io/druid/curator/discovery/ServerDiscoverySelectorTest.java b/server/src/test/java/io/druid/curator/discovery/ServerDiscoverySelectorTest.java index b45886e187b..247d8457057 100644 --- a/server/src/test/java/io/druid/curator/discovery/ServerDiscoverySelectorTest.java +++ b/server/src/test/java/io/druid/curator/discovery/ServerDiscoverySelectorTest.java @@ -28,6 +28,7 @@ import org.junit.Before; import org.junit.Test; import java.io.IOException; +import java.net.URI; public class ServerDiscoverySelectorTest { @@ -52,14 +53,62 @@ public class ServerDiscoverySelectorTest EasyMock.expect(serviceProvider.getInstance()).andReturn(instance).anyTimes(); EasyMock.expect(instance.getAddress()).andReturn(ADDRESS).anyTimes(); EasyMock.expect(instance.getPort()).andReturn(PORT).anyTimes(); - EasyMock.replay(instance,serviceProvider); + EasyMock.replay(instance, serviceProvider); Server server = serverDiscoverySelector.pick(); - Assert.assertEquals(PORT,server.getPort()); - Assert.assertEquals(ADDRESS,server.getAddress()); + Assert.assertEquals(PORT, server.getPort()); + Assert.assertEquals(ADDRESS, server.getAddress()); Assert.assertTrue(server.getHost().contains(new Integer(PORT).toString())); Assert.assertTrue(server.getHost().contains(ADDRESS)); Assert.assertEquals(new String("http"), server.getScheme()); - EasyMock.verify(instance,serviceProvider); + EasyMock.verify(instance, serviceProvider); + final URI uri = new URI(String.format("%s://%s%s", server.getScheme(), server.getHost(), "/druid/indexer/v1/action")); + Assert.assertEquals(PORT, uri.getPort()); + Assert.assertEquals(ADDRESS, uri.getHost()); + Assert.assertEquals("http", uri.getScheme()); + } + + + @Test + public void testPickIPv6() throws Exception + { + final String ADDRESS = "2001:0db8:0000:0000:0000:ff00:0042:8329"; + EasyMock.expect(serviceProvider.getInstance()).andReturn(instance).anyTimes(); + EasyMock.expect(instance.getAddress()).andReturn(ADDRESS).anyTimes(); + EasyMock.expect(instance.getPort()).andReturn(PORT).anyTimes(); + EasyMock.replay(instance, serviceProvider); + Server server = serverDiscoverySelector.pick(); + Assert.assertEquals(PORT, server.getPort()); + Assert.assertEquals(ADDRESS, server.getAddress()); + Assert.assertTrue(server.getHost().contains(new Integer(PORT).toString())); + Assert.assertTrue(server.getHost().contains(ADDRESS)); + Assert.assertEquals(new String("http"), server.getScheme()); + EasyMock.verify(instance, serviceProvider); + final URI uri = new URI(String.format("%s://%s%s", server.getScheme(), server.getHost(), "/druid/indexer/v1/action")); + Assert.assertEquals(PORT, uri.getPort()); + Assert.assertEquals(String.format("[%s]", ADDRESS), uri.getHost()); + Assert.assertEquals("http", uri.getScheme()); + } + + + @Test + public void testPickIPv6Bracket() throws Exception + { + final String ADDRESS = "[2001:0db8:0000:0000:0000:ff00:0042:8329]"; + EasyMock.expect(serviceProvider.getInstance()).andReturn(instance).anyTimes(); + EasyMock.expect(instance.getAddress()).andReturn(ADDRESS).anyTimes(); + EasyMock.expect(instance.getPort()).andReturn(PORT).anyTimes(); + EasyMock.replay(instance, serviceProvider); + Server server = serverDiscoverySelector.pick(); + Assert.assertEquals(PORT, server.getPort()); + Assert.assertEquals(ADDRESS, server.getAddress()); + Assert.assertTrue(server.getHost().contains(new Integer(PORT).toString())); + Assert.assertTrue(server.getHost().contains(ADDRESS)); + Assert.assertEquals(new String("http"), server.getScheme()); + EasyMock.verify(instance, serviceProvider); + final URI uri = new URI(String.format("%s://%s%s", server.getScheme(), server.getHost(), "/druid/indexer/v1/action")); + Assert.assertEquals(PORT, uri.getPort()); + Assert.assertEquals(ADDRESS, uri.getHost()); + Assert.assertEquals("http", uri.getScheme()); } @Test From e450877a7889a86497c8cd9be377e3433ac35997 Mon Sep 17 00:00:00 2001 From: Charles Allen Date: Thu, 8 Oct 2015 10:03:50 -0700 Subject: [PATCH 2/3] Make ServerDiscoverySelector more IPv6 friendly --- .../io/druid/curator/discovery/ServerDiscoverySelector.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/io/druid/curator/discovery/ServerDiscoverySelector.java b/server/src/main/java/io/druid/curator/discovery/ServerDiscoverySelector.java index 32f5c0906be..c94cd6cb422 100644 --- a/server/src/main/java/io/druid/curator/discovery/ServerDiscoverySelector.java +++ b/server/src/main/java/io/druid/curator/discovery/ServerDiscoverySelector.java @@ -17,6 +17,7 @@ package io.druid.curator.discovery; +import com.google.common.net.HostAndPort; import com.metamx.common.lifecycle.LifecycleStart; import com.metamx.common.lifecycle.LifecycleStop; import com.metamx.common.logger.Logger; @@ -62,7 +63,7 @@ public class ServerDiscoverySelector implements DiscoverySelector @Override public String getHost() { - return String.format("%s:%d", getAddress(), getPort()); + return HostAndPort.fromParts(getAddress(), getPort()).toString(); } @Override From bf11723a529327bb987825c88e50bece9b451736 Mon Sep 17 00:00:00 2001 From: Charles Allen Date: Mon, 12 Oct 2015 11:22:23 -0700 Subject: [PATCH 3/3] Update usages of io.druid.client.selector.Server to build URL or URI directly instead of using String.format --- .../actions/RemoteTaskActionClient.java | 2 +- .../indexing/IndexingServiceClient.java | 11 ++++- .../bridge/BridgeQuerySegmentWalker.java | 12 ++--- .../server/router/CoordinatorRuleManager.java | 14 +++++- .../ServerDiscoverySelectorTest.java | 44 ++++++++++++++----- 5 files changed, 64 insertions(+), 19 deletions(-) diff --git a/indexing-service/src/main/java/io/druid/indexing/common/actions/RemoteTaskActionClient.java b/indexing-service/src/main/java/io/druid/indexing/common/actions/RemoteTaskActionClient.java index 8f4d527f3c9..b048bf25346 100644 --- a/indexing-service/src/main/java/io/druid/indexing/common/actions/RemoteTaskActionClient.java +++ b/indexing-service/src/main/java/io/druid/indexing/common/actions/RemoteTaskActionClient.java @@ -155,7 +155,7 @@ public class RemoteTaskActionClient implements TaskActionClient private URI makeServiceUri(final Server instance) throws URISyntaxException { - return new URI(String.format("%s://%s%s", instance.getScheme(), instance.getHost(), "/druid/indexer/v1/action")); + return new URI(instance.getScheme(), null, instance.getAddress(), instance.getPort(), "/druid/indexer/v1/action", null, null); } private Server getServiceInstance() diff --git a/server/src/main/java/io/druid/client/indexing/IndexingServiceClient.java b/server/src/main/java/io/druid/client/indexing/IndexingServiceClient.java index 844bb4821cc..2f81c666b52 100644 --- a/server/src/main/java/io/druid/client/indexing/IndexingServiceClient.java +++ b/server/src/main/java/io/druid/client/indexing/IndexingServiceClient.java @@ -34,6 +34,7 @@ import org.joda.time.Interval; import javax.ws.rs.core.MediaType; import java.io.InputStream; +import java.net.URI; import java.net.URL; import java.util.Iterator; import java.util.List; @@ -115,7 +116,15 @@ public class IndexingServiceClient throw new ISE("Cannot find instance of indexingService"); } - return String.format("http://%s/druid/indexer/v1", instance.getHost()); + return new URI( + instance.getScheme(), + null, + instance.getAddress(), + instance.getPort(), + "/druid/indexer/v1", + null, + null + ).toString(); } catch (Exception e) { throw Throwables.propagate(e); diff --git a/server/src/main/java/io/druid/server/bridge/BridgeQuerySegmentWalker.java b/server/src/main/java/io/druid/server/bridge/BridgeQuerySegmentWalker.java index 48c8c4adfda..edc0c16fa83 100644 --- a/server/src/main/java/io/druid/server/bridge/BridgeQuerySegmentWalker.java +++ b/server/src/main/java/io/druid/server/bridge/BridgeQuerySegmentWalker.java @@ -95,16 +95,18 @@ public class BridgeQuerySegmentWalker implements QuerySegmentWalker if (instance == null) { return Sequences.empty(); } - - final String url = String.format( - "http://%s/druid/v2/", - brokerSelector.pick().getHost() + final Server brokerServer = brokerSelector.pick(); + final URL url = new URL( + brokerServer.getScheme(), + brokerServer.getAddress(), + brokerServer.getPort(), + "/druid/v2/" ); StatusResponseHolder response = httpClient.go( new Request( HttpMethod.POST, - new URL(url) + url ).setContent( MediaType.APPLICATION_JSON, jsonMapper.writeValueAsBytes(query) diff --git a/server/src/main/java/io/druid/server/router/CoordinatorRuleManager.java b/server/src/main/java/io/druid/server/router/CoordinatorRuleManager.java index c97d5fe48fd..ee4383c395f 100644 --- a/server/src/main/java/io/druid/server/router/CoordinatorRuleManager.java +++ b/server/src/main/java/io/druid/server/router/CoordinatorRuleManager.java @@ -42,6 +42,8 @@ import org.jboss.netty.handler.codec.http.HttpMethod; import org.jboss.netty.handler.codec.http.HttpResponseStatus; import org.joda.time.Duration; +import java.net.URI; +import java.net.URISyntaxException; import java.net.URL; import java.util.List; import java.util.Map; @@ -197,7 +199,7 @@ public class CoordinatorRuleManager return retVal; } - private String getRuleURL() + private String getRuleURL() throws URISyntaxException { Server server = selector.pick(); @@ -206,6 +208,14 @@ public class CoordinatorRuleManager return null; } - return String.format("http://%s%s", server.getHost(), config.get().getRulesEndpoint()); + return new URI( + server.getScheme(), + null, + server.getAddress(), + server.getPort(), + config.get().getRulesEndpoint(), + null, + null + ).toString(); } } diff --git a/server/src/test/java/io/druid/curator/discovery/ServerDiscoverySelectorTest.java b/server/src/test/java/io/druid/curator/discovery/ServerDiscoverySelectorTest.java index 247d8457057..514e71036f0 100644 --- a/server/src/test/java/io/druid/curator/discovery/ServerDiscoverySelectorTest.java +++ b/server/src/test/java/io/druid/curator/discovery/ServerDiscoverySelectorTest.java @@ -34,7 +34,7 @@ public class ServerDiscoverySelectorTest { private ServiceProvider serviceProvider; - private ServerDiscoverySelector serverDiscoverySelector; + private ServerDiscoverySelector serverDiscoverySelector; private ServiceInstance instance; private static final int PORT = 8080; private static final String ADDRESS = "localhost"; @@ -57,11 +57,19 @@ public class ServerDiscoverySelectorTest Server server = serverDiscoverySelector.pick(); Assert.assertEquals(PORT, server.getPort()); Assert.assertEquals(ADDRESS, server.getAddress()); - Assert.assertTrue(server.getHost().contains(new Integer(PORT).toString())); + Assert.assertTrue(server.getHost().contains(Integer.toString(PORT))); Assert.assertTrue(server.getHost().contains(ADDRESS)); - Assert.assertEquals(new String("http"), server.getScheme()); + Assert.assertEquals("http", server.getScheme()); EasyMock.verify(instance, serviceProvider); - final URI uri = new URI(String.format("%s://%s%s", server.getScheme(), server.getHost(), "/druid/indexer/v1/action")); + final URI uri = new URI( + server.getScheme(), + null, + server.getAddress(), + server.getPort(), + "/druid/indexer/v1/action", + null, + null + ); Assert.assertEquals(PORT, uri.getPort()); Assert.assertEquals(ADDRESS, uri.getHost()); Assert.assertEquals("http", uri.getScheme()); @@ -79,11 +87,19 @@ public class ServerDiscoverySelectorTest Server server = serverDiscoverySelector.pick(); Assert.assertEquals(PORT, server.getPort()); Assert.assertEquals(ADDRESS, server.getAddress()); - Assert.assertTrue(server.getHost().contains(new Integer(PORT).toString())); + Assert.assertTrue(server.getHost().contains(Integer.toString(PORT))); Assert.assertTrue(server.getHost().contains(ADDRESS)); - Assert.assertEquals(new String("http"), server.getScheme()); + Assert.assertEquals("http", server.getScheme()); EasyMock.verify(instance, serviceProvider); - final URI uri = new URI(String.format("%s://%s%s", server.getScheme(), server.getHost(), "/druid/indexer/v1/action")); + final URI uri = new URI( + server.getScheme(), + null, + server.getAddress(), + server.getPort(), + "/druid/indexer/v1/action", + null, + null + ); Assert.assertEquals(PORT, uri.getPort()); Assert.assertEquals(String.format("[%s]", ADDRESS), uri.getHost()); Assert.assertEquals("http", uri.getScheme()); @@ -101,11 +117,19 @@ public class ServerDiscoverySelectorTest Server server = serverDiscoverySelector.pick(); Assert.assertEquals(PORT, server.getPort()); Assert.assertEquals(ADDRESS, server.getAddress()); - Assert.assertTrue(server.getHost().contains(new Integer(PORT).toString())); + Assert.assertTrue(server.getHost().contains(Integer.toString(PORT))); Assert.assertTrue(server.getHost().contains(ADDRESS)); - Assert.assertEquals(new String("http"), server.getScheme()); + Assert.assertEquals("http", server.getScheme()); EasyMock.verify(instance, serviceProvider); - final URI uri = new URI(String.format("%s://%s%s", server.getScheme(), server.getHost(), "/druid/indexer/v1/action")); + final URI uri = new URI( + server.getScheme(), + null, + server.getAddress(), + server.getPort(), + "/druid/indexer/v1/action", + null, + null + ); Assert.assertEquals(PORT, uri.getPort()); Assert.assertEquals(ADDRESS, uri.getHost()); Assert.assertEquals("http", uri.getScheme());