[druid] branch master updated: Make leader redirection work when both plainText and TLS ports are set (#13847)

[ab...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

abhishek pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/druid.git


The following commit(s) were added to refs/heads/master by this push:
     new 48f4330100 Make leader redirection work when both plainText and TLS ports are set (#13847)
48f4330100 is described below

commit 48f4330100e73be5dbf85c3002eaf0d4e5224e63
Author: Abhishek Agarwal <14...@users.noreply.github.com>
AuthorDate: Sun Feb 26 21:23:29 2023 +0530

    Make leader redirection work when both plainText and TLS ports are set (#13847)
    
    When both plainText and TLS ports are set in druid, the redirection to a different leader node can fail. This is caused by how we compare a redirect path and the leader locations registered with a druid node. While the registered location has both plainText and TLS port set, the redirect path only has one port since it's a URI.
---
 .../java/org/apache/druid/rpc/ServiceClientImpl.java | 19 +++++++++++++------
 .../org/apache/druid/rpc/ServiceClientImplTest.java  | 20 ++++++++++++++++++++
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/server/src/main/java/org/apache/druid/rpc/ServiceClientImpl.java b/server/src/main/java/org/apache/druid/rpc/ServiceClientImpl.java
index eca2cfdc5a..98191b3e13 100644
--- a/server/src/main/java/org/apache/druid/rpc/ServiceClientImpl.java
+++ b/server/src/main/java/org/apache/druid/rpc/ServiceClientImpl.java
@@ -301,8 +301,7 @@ public class ServiceClientImpl implements ServiceClient
                     );
                   } else if (serviceLocations.getLocations()
                                              .stream()
-                                             .anyMatch(loc -> serviceLocationNoPath(loc)
-                                                 .equals(redirectLocationNoPath))) {
+                                             .anyMatch(loc -> serviceLocationMatches(loc, redirectLocationNoPath))) {
                     // Valid redirect, to a server that is one of the known locations.
                     final boolean isRedirectLoop = redirectLocations.contains(newUri);
                     final boolean isRedirectChainTooLong = redirectLocations.size() >= MAX_REDIRECTS;
@@ -415,7 +414,7 @@ public class ServiceClientImpl implements ServiceClient
     if (preferred != null) {
       // Preferred location is set. Use it if it's one of known locations.
       for (final ServiceLocation location : locations.getLocations()) {
-        if (serviceLocationNoPath(location).equals(preferred)) {
+        if (serviceLocationMatches(location, preferred)) {
           return location;
         }
       }
@@ -511,11 +510,19 @@ public class ServiceClientImpl implements ServiceClient
   }
 
   /**
-   * Returns a {@link ServiceLocation} without its path.
+   * Returns true if two service locations are same or false otherwise. If a port is negative, we ignore that
+   * port for comparison.
    */
-  static ServiceLocation serviceLocationNoPath(final ServiceLocation location)
+  static boolean serviceLocationMatches(final ServiceLocation left, final ServiceLocation right)
   {
-    return new ServiceLocation(location.getHost(), location.getPlaintextPort(), location.getTlsPort(), "");
+    return left.getHost().equals(right.getHost())
+        && portMatches(left.getPlaintextPort(), right.getPlaintextPort())
+        && portMatches(left.getTlsPort(), right.getTlsPort());
+  }
+
+  static boolean portMatches(int left, int right)
+  {
+    return left < 0 || right < 0 || left == right;
   }
 
   @VisibleForTesting
diff --git a/server/src/test/java/org/apache/druid/rpc/ServiceClientImplTest.java b/server/src/test/java/org/apache/druid/rpc/ServiceClientImplTest.java
index dc8bba87d1..20487aeac8 100644
--- a/server/src/test/java/org/apache/druid/rpc/ServiceClientImplTest.java
+++ b/server/src/test/java/org/apache/druid/rpc/ServiceClientImplTest.java
@@ -70,6 +70,8 @@ public class ServiceClientImplTest
   private static final ServiceLocation SERVER3 = new ServiceLocation("example.com", -1, 1111, "/q");
   private static final ServiceLocation SERVER4 = new ServiceLocation("example.com", -1, 2222, "/q");
   private static final ServiceLocation SERVER5 = new ServiceLocation("example.com", -1, 3333, "/q");
+  private static final ServiceLocation SERVER6 = new ServiceLocation("mixed.com", 201, 111, "/q");
+  private static final ServiceLocation SERVER7 = new ServiceLocation("mixed.com", 203, 222, "/q");
 
   private ScheduledExecutorService exec;
 
@@ -272,6 +274,24 @@ public class ServiceClientImplTest
     Assert.assertEquals(expectedResponseObject, response);
   }
 
+  @Test
+  public void test_request_followRedirect_mixedPorts() throws Exception
+  {
+    final RequestBuilder requestBuilder = new RequestBuilder(HttpMethod.GET, "/foo");
+    final ImmutableMap<String, String> expectedResponseObject = ImmutableMap.of("foo", "bar");
+
+    // Redirect from SERVER6 -> SERVER7.
+    stubLocatorCall(locations(SERVER6, SERVER7));
+    expectHttpCall(requestBuilder, SERVER6)
+        .thenReturn(redirectResponse(requestBuilder.build(SERVER7).getUrl().toString()));
+    expectHttpCall(requestBuilder, SERVER7).thenReturn(valueResponse(expectedResponseObject));
+
+    serviceClient = makeServiceClient(StandardRetryPolicy.noRetries());
+    final Map<String, String> response = doRequest(serviceClient, requestBuilder);
+
+    Assert.assertEquals(expectedResponseObject, response);
+  }
+
   @Test
   public void test_request_tooLongRedirectChain()
   {


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org