finagle-core: Treat failures consistently when resolving & binding un…

…ion paths

Problem:
The name resolver ignores failures when resolving unions whereas the
name binder treats a single failure as terminal. This inconsistency is
dangerous - if a single resolution in a union flaps between successful and
 failed, the entire serverset will be emptied out leading to 100% failure.

For example, with the path (/a & /b), if /b flaps into a failed binding
state, the overall resolution will flap into failed and the serverset
will empty out.

Solution:
Both name resolution and binding should filter out failures when building a
union. In the example above, if /b flaps to a failed binding,
the union will resolve to just /a.

Differential Revision: https://phabricator.twitter.biz/D315282

CHANGELOG.rst

@@ -31,6 +31,9 @@ Runtime Behavior Changes
 * finagle-core: Request logging now defaults to disabled. Enable it by configuring the
   `RequestLogger` Stack parameter on your `Client` or `Server`. ``PHAB_ID=D308476``
 
+* finagle-core: Subtree binding failures in `NameTree.Union`'s are ignored in the
+  final binding result. ``PHAB_ID=D315282``
+
 Breaking API Changes
 ~~~~~~~~~~~~~~~~~~~~
 

finagle-core/src/main/scala/com/twitter/finagle/NameTree.scala

@@ -185,8 +185,7 @@ object NameTree {
           case Nil => accum
           case Seq(Weighted(w, tree), tail @ _*) =>
             simplify(tree) match {
-              case Fail => unionFail
-              case Neg => loop(tail, accum)
+              case Fail | Neg => loop(tail, accum)
               case tree => loop(tail, accum :+ Weighted(w, tree))
             }
         }
@@ -269,8 +268,7 @@ object NameTree {
             }
           case Seq(Weighted(_, head), tail @ _*) =>
             eval(head) match {
-              case Fail => Fail
-              case Neg => loop(tail, accum)
+              case Fail | Neg => loop(tail, accum)
               case Leaf(value) => loop(tail, accum :+ value)
               case _ => scala.sys.error("bug")
             }

finagle-core/src/test/scala/com/twitter/finagle/NameTreeTest.scala

@@ -157,7 +157,8 @@ class NameTreeTest extends FunSuite {
       "~ | (~ | $) | /blah" -> Some(Set.empty),
       "(~|$|/foo) & (/bar|/blah) & ~ & /FOO" -> Some(Set("/bar", "/FOO")),
       "! | /ok" -> None,
-      "/ok & !" -> None,
+      "/ok & !" -> Some(Set("/ok")),
+      "! & ! & !" -> None,
       "~ | /ok | !" -> Some(Set("/ok"))
     )