JAVA-2527: Allow AllNodesFailedException to accept more than one error per node (apache#1362)

Author: adutra

changelog/README.md

@@ -4,6 +4,7 @@
 
 ### 4.4.0 (in progress)
 
+- [bug] JAVA-2527: Allow AllNodesFailedException to accept more than one error per node
 - [improvement] JAVA-2546: Abort schema refresh if a query fails
 
 ### 4.3.0

core/src/main/java/com/datastax/oss/driver/api/core/AllNodesFailedException.java

@@ -17,87 +17,158 @@
 
 import com.datastax.oss.driver.api.core.cql.ExecutionInfo;
 import com.datastax.oss.driver.api.core.metadata.Node;
-import com.datastax.oss.driver.shaded.guava.common.base.Joiner;
+import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableList;
 import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableMap;
 import com.datastax.oss.driver.shaded.guava.common.collect.Iterables;
 import edu.umd.cs.findbugs.annotations.NonNull;
 import edu.umd.cs.findbugs.annotations.Nullable;
+import java.util.ArrayList;
+import java.util.Iterator;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 
 /**
  * Thrown when a query failed on all the coordinators it was tried on. This exception may wrap
  * multiple errors, use {@link #getErrors()} to inspect the individual problem on each node.
  */
 public class AllNodesFailedException extends DriverException {
 
+  /** @deprecated Use {@link #fromErrors(List)} instead. */
   @NonNull
+  @Deprecated
   public static AllNodesFailedException fromErrors(@Nullable Map<Node, Throwable> errors) {
     if (errors == null || errors.isEmpty()) {
       return new NoNodeAvailableException();
     } else {
-      return new AllNodesFailedException(ImmutableMap.copyOf(errors));
+      return new AllNodesFailedException(groupByNode(errors));
     }
   }
 
   @NonNull
-  public static AllNodesFailedException fromErrors(
-      @Nullable List<Map.Entry<Node, Throwable>> errors) {
-    Map<Node, Throwable> map;
+  public static AllNodesFailedException fromErrors(@Nullable List<Entry<Node, Throwable>> errors) {
     if (errors == null || errors.isEmpty()) {
-      map = null;
+      return new NoNodeAvailableException();
     } else {
-      ImmutableMap.Builder<Node, Throwable> builder = ImmutableMap.builder();
-      for (Map.Entry<Node, Throwable> entry : errors) {
-        builder.put(entry);
-      }
-      map = builder.build();
+      return new AllNodesFailedException(groupByNode(errors));
     }
-    return fromErrors(map);
   }
 
-  private final Map<Node, Throwable> errors;
+  private final Map<Node, List<Throwable>> errors;
 
+  /** @deprecated Use {@link #AllNodesFailedException(String, ExecutionInfo, Iterable)} instead. */
+  @Deprecated
   protected AllNodesFailedException(
       @NonNull String message,
       @Nullable ExecutionInfo executionInfo,
       @NonNull Map<Node, Throwable> errors) {
     super(message, executionInfo, null, true);
-    this.errors = errors;
+    this.errors = toDeepImmutableMap(groupByNode(errors));
   }
 
-  private AllNodesFailedException(Map<Node, Throwable> errors) {
+  protected AllNodesFailedException(
+      @NonNull String message,
+      @Nullable ExecutionInfo executionInfo,
+      @NonNull Iterable<Entry<Node, List<Throwable>>> errors) {
+    super(message, executionInfo, null, true);
+    this.errors = toDeepImmutableMap(errors);
+  }
+
+  private AllNodesFailedException(Map<Node, List<Throwable>> errors) {
     this(
         buildMessage(
             String.format("All %d node(s) tried for the query failed", errors.size()), errors),
         null,
-        errors);
+        errors.entrySet());
   }
 
-  private static String buildMessage(String baseMessage, Map<Node, Throwable> errors) {
+  private static String buildMessage(String baseMessage, Map<Node, List<Throwable>> errors) {
     int limit = Math.min(errors.size(), 3);
-    String details =
-        Joiner.on(", ").withKeyValueSeparator(": ").join(Iterables.limit(errors.entrySet(), limit));
-
+    Iterator<Entry<Node, List<Throwable>>> iterator =
+        Iterables.limit(errors.entrySet(), limit).iterator();
+    StringBuilder details = new StringBuilder();
+    while (iterator.hasNext()) {
+      Entry<Node, List<Throwable>> entry = iterator.next();
+      details.append(entry.getKey()).append(": ").append(entry.getValue());
+      if (iterator.hasNext()) {
+        details.append(", ");
+      }
+    }
     return String.format(
-        baseMessage + " (showing first %d, use getErrors() for more: %s)", limit, details);
+        "%s (showing first %d nodes, use getAllErrors() for more): %s",
+        baseMessage, limit, details);
   }
 
-  /** The details of the individual error on each node. */
+  /**
+   * An immutable map containing the first error on each tried node.
+   *
+   * @deprecated Use {@link #getAllErrors()} instead.
+   */
   @NonNull
+  @Deprecated
   public Map<Node, Throwable> getErrors() {
+    ImmutableMap.Builder<Node, Throwable> builder = ImmutableMap.builder();
+    for (Node node : errors.keySet()) {
+      List<Throwable> nodeErrors = errors.get(node);
+      if (!nodeErrors.isEmpty()) {
+        builder.put(node, nodeErrors.get(0));
+      }
+    }
+    return builder.build();
+  }
+
+  /** An immutable map containing all errors on each tried node. */
+  @NonNull
+  public Map<Node, List<Throwable>> getAllErrors() {
     return errors;
   }
 
   @NonNull
   @Override
   public DriverException copy() {
-    return new AllNodesFailedException(getMessage(), getExecutionInfo(), errors);
+    return new AllNodesFailedException(getMessage(), getExecutionInfo(), errors.entrySet());
   }
 
   @NonNull
   public AllNodesFailedException reword(String newMessage) {
     return new AllNodesFailedException(
-        buildMessage(newMessage, errors), getExecutionInfo(), errors);
+        buildMessage(newMessage, errors), getExecutionInfo(), errors.entrySet());
+  }
+
+  private static Map<Node, List<Throwable>> groupByNode(Map<Node, Throwable> errors) {
+    return groupByNode(errors.entrySet());
+  }
+
+  private static Map<Node, List<Throwable>> groupByNode(Iterable<Entry<Node, Throwable>> errors) {
+    // no need for immutable collections here
+    Map<Node, List<Throwable>> map = new LinkedHashMap<>();
+    for (Entry<Node, Throwable> entry : errors) {
+      Node node = entry.getKey();
+      Throwable error = entry.getValue();
+      map.compute(
+          node,
+          (k, v) -> {
+            if (v == null) {
+              v = new ArrayList<>();
+            }
+            v.add(error);
+            return v;
+          });
+    }
+    return map;
+  }
+
+  private static Map<Node, List<Throwable>> toDeepImmutableMap(Map<Node, List<Throwable>> errors) {
+    return toDeepImmutableMap(errors.entrySet());
+  }
+
+  private static Map<Node, List<Throwable>> toDeepImmutableMap(
+      Iterable<Entry<Node, List<Throwable>>> errors) {
+    ImmutableMap.Builder<Node, List<Throwable>> builder = ImmutableMap.builder();
+    for (Entry<Node, List<Throwable>> entry : errors) {
+      builder.put(entry.getKey(), ImmutableList.copyOf(entry.getValue()));
+    }
+    return builder.build();
   }
 }

core/src/main/java/com/datastax/oss/driver/api/core/DriverException.java

@@ -74,8 +74,8 @@ protected DriverException(
    *
    * <p>Note that this is only set for exceptions that are rethrown directly to the client from a
    * session call. For example, individual node errors stored in {@link
-   * AllNodesFailedException#getErrors()} or {@link ExecutionInfo#getErrors()} do not contain their
-   * own execution info, and therefore return null from this method.
+   * AllNodesFailedException#getAllErrors()} or {@link ExecutionInfo#getErrors()} do not contain
+   * their own execution info, and therefore return null from this method.
    *
    * <p>It will also be null if you serialize and deserialize an exception.
    */

core/src/main/java/com/datastax/oss/driver/api/core/NoNodeAvailableException.java

@@ -31,7 +31,7 @@ public NoNodeAvailableException() {
   }
 
   private NoNodeAvailableException(ExecutionInfo executionInfo) {
-    super("No node was available to execute the query", executionInfo, Collections.emptyMap());
+    super("No node was available to execute the query", executionInfo, Collections.emptySet());
   }
 
   @NonNull

core/src/main/java/com/datastax/oss/driver/internal/core/control/ControlConnection.java

@@ -48,9 +48,12 @@
 import com.datastax.oss.protocol.internal.response.event.TopologyChangeEvent;
 import edu.umd.cs.findbugs.annotations.NonNull;
 import io.netty.util.concurrent.EventExecutor;
+import java.util.AbstractMap.SimpleEntry;
+import java.util.ArrayList;
 import java.util.Collection;
-import java.util.LinkedHashMap;
+import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Queue;
 import java.util.WeakHashMap;
 import java.util.concurrent.CompletableFuture;
@@ -348,7 +351,7 @@ private CompletionStage<Boolean> reconnect() {
 
     private void connect(
         Queue<Node> nodes,
-        Map<Node, Throwable> errors,
+        List<Entry<Node, Throwable>> errors,
         Runnable onSuccess,
         Consumer<Throwable> onFailure) {
       assert adminExecutor.inEventLoop();
@@ -390,9 +393,9 @@ private void connect(
                                 error);
                           }
                         }
-                        Map<Node, Throwable> newErrors =
-                            (errors == null) ? new LinkedHashMap<>() : errors;
-                        newErrors.put(node, error);
+                        List<Entry<Node, Throwable>> newErrors =
+                            (errors == null) ? new ArrayList<>() : errors;
+                        newErrors.add(new SimpleEntry<>(node, error));
                         context.getEventBus().fire(ChannelEvent.controlConnectionFailed(node));
                         connect(nodes, newErrors, onSuccess, onFailure);
                       }
@@ -573,20 +576,21 @@ private void forceClose() {
   }
 
   private boolean isAuthFailure(Throwable error) {
-    boolean authFailure = true;
     if (error instanceof AllNodesFailedException) {
-      Collection<Throwable> errors = ((AllNodesFailedException) error).getErrors().values();
+      Collection<List<Throwable>> errors =
+          ((AllNodesFailedException) error).getAllErrors().values();
       if (errors.size() == 0) {
         return false;
       }
-      for (Throwable nodeError : errors) {
-        if (!(nodeError instanceof AuthenticationException)) {
-          authFailure = false;
-          break;
+      for (List<Throwable> nodeErrors : errors) {
+        for (Throwable nodeError : nodeErrors) {
+          if (!(nodeError instanceof AuthenticationException)) {
+            return false;
+          }
         }
       }
     }
-    return authFailure;
+    return true;
   }
 
   private static ImmutableList<String> buildEventTypes(boolean listenClusterEvents) {

core/src/test/java/com/datastax/oss/driver/api/core/AllNodesFailedExceptionTest.java

@@ -0,0 +1,93 @@
+/*
+ * Copyright DataStax, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.datastax.oss.driver.api.core;
+
+import static com.datastax.oss.driver.api.core.ConsistencyLevel.QUORUM;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.data.MapEntry.entry;
+
+import com.datastax.oss.driver.api.core.metadata.Node;
+import com.datastax.oss.driver.api.core.servererrors.ReadTimeoutException;
+import com.datastax.oss.driver.api.core.servererrors.UnavailableException;
+import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableList;
+import com.datastax.oss.driver.shaded.guava.common.collect.ImmutableMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+@RunWith(MockitoJUnitRunner.class)
+public class AllNodesFailedExceptionTest {
+
+  @Mock(name = "node1")
+  private Node node1;
+
+  @Mock(name = "node2")
+  private Node node2;
+
+  @SuppressWarnings("deprecation")
+  @Test
+  public void should_create_instance_from_map_of_first_errors() {
+    // given
+    UnavailableException e1 = new UnavailableException(node1, QUORUM, 2, 1);
+    ReadTimeoutException e2 = new ReadTimeoutException(node2, QUORUM, 2, 1, false);
+    Map<Node, Throwable> errors = ImmutableMap.of(node1, e1, node2, e2);
+    // when
+    AllNodesFailedException e = AllNodesFailedException.fromErrors(errors);
+    // then
+    assertThat(e)
+        .hasMessage(
+            "All 2 node(s) tried for the query failed "
+                + "(showing first 2 nodes, use getAllErrors() for more): "
+                + "node1: [%s], node2: [%s]",
+            e1, e2);
+    assertThat(e.getAllErrors())
+        .hasEntrySatisfying(node1, list -> assertThat(list).containsExactly(e1));
+    assertThat(e.getAllErrors())
+        .hasEntrySatisfying(node2, list -> assertThat(list).containsExactly(e2));
+    assertThat(e.getErrors()).containsEntry(node1, e1);
+    assertThat(e.getErrors()).containsEntry(node2, e2);
+  }
+
+  @SuppressWarnings("deprecation")
+  @Test
+  public void should_create_instance_from_list_of_all_errors() {
+    // given
+    UnavailableException e1a = new UnavailableException(node1, QUORUM, 2, 1);
+    ReadTimeoutException e1b = new ReadTimeoutException(node1, QUORUM, 2, 1, false);
+    ReadTimeoutException e2a = new ReadTimeoutException(node2, QUORUM, 2, 1, false);
+    List<Entry<Node, Throwable>> errors =
+        ImmutableList.of(entry(node1, e1a), entry(node1, e1b), entry(node2, e2a));
+    // when
+    AllNodesFailedException e = AllNodesFailedException.fromErrors(errors);
+    // then
+    assertThat(e)
+        .hasMessage(
+            "All 2 node(s) tried for the query failed "
+                + "(showing first 2 nodes, use getAllErrors() for more): "
+                + "node1: [%s, %s], node2: [%s]",
+            e1a, e1b, e2a);
+    assertThat(e.getAllErrors())
+        .hasEntrySatisfying(node1, list -> assertThat(list).containsExactly(e1a, e1b));
+    assertThat(e.getAllErrors())
+        .hasEntrySatisfying(node2, list -> assertThat(list).containsExactly(e2a));
+    assertThat(e.getErrors()).containsEntry(node1, e1a);
+    assertThat(e.getErrors()).containsEntry(node2, e2a);
+  }
+}