From 229db51eb42853bf7a8e23195fd66c1e71771e35 Mon Sep 17 00:00:00 2001
From: Patrik Nordwall <patrik.nordwall@gmail.com>
Date: Mon, 10 Dec 2018 17:18:53 +0100
Subject: [PATCH] Cleanup validNodeForGossip (#25965)

The intended behavior is:
* don't gossip to node marked as unreachable by self (heartbeat
  messages are not getting through so no point in trying to gossip).
* gossip is allowed to nodes marked as unreachable by others

This doesn't change anything from how it worked before, but I think the
original intention before the multi-dc changes was to not gossip to
unreachable at all no matter who marked them.
---
 .../scala/akka/cluster/MembershipState.scala  | 10 ++++++----
 .../cluster/GossipTargetSelectorSpec.scala    | 20 +++++++++++++++++++
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/akka-cluster/src/main/scala/akka/cluster/MembershipState.scala b/akka-cluster/src/main/scala/akka/cluster/MembershipState.scala
index b5e82380d6..56bc63aee4 100644
--- a/akka-cluster/src/main/scala/akka/cluster/MembershipState.scala
+++ b/akka-cluster/src/main/scala/akka/cluster/MembershipState.scala
@@ -168,11 +168,13 @@ import scala.util.Random
   def isInSameDc(node: UniqueAddress): Boolean =
     node == selfUniqueAddress || latestGossip.member(node).dataCenter == selfDc
 
+  /**
+   * Never gossip to self and not to node marked as unreachable by self (heartbeat
+   * messages are not getting through so no point in trying to gossip).
+   * Nodes marked as unreachable by others are still valid targets for gossip.
+   */
   def validNodeForGossip(node: UniqueAddress): Boolean =
-    node != selfUniqueAddress &&
-      ((isInSameDc(node) && isReachableExcludingDownedObservers(node)) ||
-        // if cross DC we need to check pairwise unreachable observation
-        overview.reachability.isReachable(selfUniqueAddress, node))
+    node != selfUniqueAddress && overview.reachability.isReachable(selfUniqueAddress, node)
 
   def youngestMember: Member = {
     val mbrs = dcMembers
diff --git a/akka-cluster/src/test/scala/akka/cluster/GossipTargetSelectorSpec.scala b/akka-cluster/src/test/scala/akka/cluster/GossipTargetSelectorSpec.scala
index ae5989d825..8e65da45eb 100644
--- a/akka-cluster/src/test/scala/akka/cluster/GossipTargetSelectorSpec.scala
+++ b/akka-cluster/src/test/scala/akka/cluster/GossipTargetSelectorSpec.scala
@@ -118,6 +118,26 @@ class GossipTargetSelectorSpec extends WordSpec with Matchers {
       gossipTo should ===(Vector[UniqueAddress](cDc1))
     }
 
+    "select among unreachable nodes if marked as unreachable by someone else" in {
+      val alwaysLocalSelector = new GossipTargetSelector(400, 0.2) {
+        override protected def preferNodesWithDifferentView(s: MembershipState): Boolean = false
+      }
+
+      val state = MembershipState(
+        Gossip(
+          members = SortedSet(aDc1, bDc1, cDc1),
+          overview = GossipOverview(
+            reachability = Reachability.empty.unreachable(aDc1, bDc1).unreachable(bDc1, cDc1))),
+        aDc1,
+        aDc1.dataCenter,
+        crossDcConnections = 5)
+      val gossipTo = alwaysLocalSelector.gossipTargets(state)
+
+      // a1 marked b as unreachable so will not pick b
+      // b marked c as unreachable so that is ok as target
+      gossipTo should ===(Vector[UniqueAddress](cDc1))
+    }
+
     "continue with the next dc when doing cross dc and no node where suitable" in {
       val selector = new GossipTargetSelector(400, 0.2) {
         override protected def selectDcLocalNodes(s: MembershipState): Boolean = false