[finagle-core] Remove Namer trait from Dtab

Problem
--

We have multiple ways to bind a name via a Dtab: Dtab itself
is a `Namer`, and `NameInterpreter` convert a Path to a bound
name, taking the Dtab into account.

Solution
--

Make local dtab resolution a NameInterpreter.

Result
--

One mechanism for resolving a path to a bound name using the
dtab.

RB_ID=711681
TBR=true

CHANGES

@@ -32,6 +32,11 @@ Breaking API Changes
     HttpRequest, and converted the rest of the public API to not leak
     other Netty types (notably ChannelBuffer is replaced by Buf). ``RB_ID=695896``
 
+  * finagle-core: Dtab does not implement the Namer interface anymore. Use
+    `c.t.f.naming.DefaultInterpreter` to bind a name via a Dtab. Support for Dtab entries starting
+    with /#/ has been removed. `c.t.f.Namer.bindAndEval` has been removed. Use
+    `c.t.f.Namer.resolve` instead. ``RB_ID=711681``
+
 6.26.0
 ~~~~~~~
 

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

@@ -1,56 +1,46 @@
 package com.twitter.finagle
 
 import com.twitter.app.Flaggable
-import com.twitter.util.{Activity, Local, Var}
+import com.twitter.util.Local
 import java.io.PrintWriter
-import java.net.SocketAddress
-import java.util.concurrent.atomic.AtomicReference
 import scala.collection.generic.CanBuildFrom
 import scala.collection.immutable.VectorBuilder
 import scala.collection.mutable.Builder
-import scala.collection.mutable
 
 
 /**
- * A Dtab--short for delegation table--comprises a sequence
- * of delegation rules. Together, these describe how to bind a
- * path to an Addr.
+ * A Dtab--short for delegation table--comprises a sequence of
+ * delegation rules. Together, these describe how to bind a
+ * [[com.twitter.finagle.Path]] to a set of
+ * [[com.twitter.finagle.Addr]]. [[com.twitter.finagle.naming.DefaultInterpreter]]
+ * implements the default binding stategy.
  */
 case class Dtab(dentries0: IndexedSeq[Dentry])
-    extends IndexedSeq[Dentry] with Namer {
+  extends IndexedSeq[Dentry] {
+
   private lazy val dentries = dentries0.reverse
 
   def apply(i: Int): Dentry = dentries0(i)
   def length = dentries0.length
   override def isEmpty = length == 0
 
-  private[this] object NamerPath {
-    def unapply(path: Path): Option[(Namer, Path)] = path match {
-      case Path.Utf8("#", kind, rest@_*) => Some((Namer.namerOfKind(kind), Path.Utf8(rest: _*)))
-      case _ => None
-    }
-  }
-
-  private def lookup0(path: Path): NameTree[Path] = {
-    val matches = dentries collect {
-      case Dentry(prefix, dst) if path startsWith prefix =>
-        val suff = path drop prefix.size
-        dst map { pfx => pfx ++ suff }
+  /**
+   * Lookup the given `path` with this dtab.
+   */
+  def lookup(path: Path): NameTree[Name.Path] = {
+    val matches = dentries.collect {
+      case Dentry(prefix, dst) if path.startsWith(prefix) =>
+        val suff = path.drop(prefix.size)
+        dst.map { pfx => Name.Path(pfx ++ suff) }
     }
 
     matches.size match {
       case 0 => NameTree.Neg
-      case 1 => matches(0)
+      case 1 => matches.head
       case _ => NameTree.Alt(matches:_*)
     }
   }
 
-  def lookup(path: Path): Activity[NameTree[Name]] =
-    path match {
-      case NamerPath(namer, rest) => namer.lookup(rest)
-      case _ => Activity.value(lookup0(path) map { path => Name(path) })
-    }
-
   /**
    * Construct a new Dtab with the given delegation
    * entry appended.
@@ -113,7 +103,7 @@ case class Dtab(dentries0: IndexedSeq[Dentry])
    * whose destination name trees have been simplified. The returned
    * Dtab is equivalent with respect to evaluation.
    *
-   * @todo dedup equivalent entries so that the only the last entry is retained
+   * @todo dedup equivalent entries so that only the last entry is retained
    * @todo collapse entries with common prefixes
    */
   def simplified: Dtab = Dtab({
@@ -130,10 +120,9 @@ case class Dtab(dentries0: IndexedSeq[Dentry])
 }
 
 /**
- * Trait Dentry describes a delegation table entry.
- * It always has a prefix, describing the paths to
- * which the entry applies, and a bind method to
- * bind the given path.
+ * Trait Dentry describes a delegation table entry. `prefix` describes
+ * the paths that the entry applies to. `dst` describes the resulting
+ * tree for this prefix on lookup.
  */
 case class Dentry(prefix: Path, dst: NameTree[Path]) {
   def show = "%s=>%s".format(prefix.show, dst.show)
@@ -147,7 +136,7 @@ object Dentry {
    * dentry     ::= path '=>' tree
    * }}}
    *
-   * where the productions ``path`` and ``tree`` are from the grammar
+   * where the productions `path` and `tree` are from the grammar
    * documented in [[com.twitter.finagle.NameTree$ NameTree.read]].
    */
   def read(s: String): Dentry = NameTreeParsers.parseDentry(s)
@@ -190,10 +179,10 @@ object Dtab {
    * every request in this process. It is generally set at process
    * startup, and not changed thereafter.
    */
-  @volatile var base: Dtab = Dtab.read("/=>/#/com.twitter.finagle.namer.global")
+  @volatile var base: Dtab = empty
 
   /**
-   * Java API for ``base_=``
+   * Java API for `base_=`
    */
   def setBase(dtab: Dtab) { base = dtab }
 
@@ -203,14 +192,14 @@ object Dtab {
    * The local, or "per-request", delegation table applies to the
    * current [[com.twitter.util.Local Local]] scope which is usually
    * defined on a per-request basis. Finagle uses the Dtab
-   * ``Dtab.base ++ Dtab.local`` to bind
-   * [[com.twitter.finagle.Name.Path Paths]].
+   * `Dtab.base ++ Dtab.local` to bind [[com.twitter.finagle.Name.Path
+   * Paths]] via a [[com.twitter.finagle.naming.NameInterpreter]].
    *
    * Local's scope is dictated by [[com.twitter.util.Local Local]].
    *
    * The local dtab is serialized into outbound requests when
    * supported protocols are used. (Http, Thrift via TTwitter, Mux,
-   * and ThriftMux are among these.) The upshot is that ``local`` is
+   * and ThriftMux are among these.) The upshot is that `local` is
    * defined for the entire request graph, so that a local dtab
    * defined here will apply to downstream services as well.
    */
@@ -221,7 +210,7 @@ object Dtab {
   def local_=(dtab: Dtab) { l() = dtab }
 
   /**
-   * Java API for ``local_=``
+   * Java API for `local_=`
    */
   def setLocal(dtab: Dtab) { local = dtab }
 
@@ -237,7 +226,7 @@ object Dtab {
    * dtab       ::= dentry ';' dtab | dentry
    * }}}
    *
-   * where the production ``dentry`` is from the grammar documented in
+   * where the production `dentry` is from the grammar documented in
    * [[com.twitter.finagle.Dentry$ Dentry.read]]
    *
    */

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

@@ -1,10 +1,7 @@
 package com.twitter.finagle
 
-import com.twitter.util.{Activity, Var}
 import com.twitter.finagle.util.Showable
 import scala.annotation.tailrec
-import java.net.{InetSocketAddress, SocketAddress}
-import scala.collection.breakOut
 
 /**
  * Name trees represent a composite T-typed name whose interpretation
@@ -323,7 +320,7 @@ object NameTree {
    * Alt(Union(Leaf(Path(foo)),Leaf(Path(bar))),Leaf(Path(baz)),Empty)
    * }}}
    *
-   * The production ``path`` is documented at [[com.twitter.finagle.Path$ Path.read]].
+   * The production `path` is documented at [[com.twitter.finagle.Path$ Path.read]].
    *
    * @throws IllegalArgumentException when the string does not
    * represent a valid name tree.

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

@@ -28,13 +28,7 @@ trait Namer { self =>
    * to 100 is allowed.
    */
   def bind(tree: NameTree[Path]): Activity[NameTree[Name.Bound]] =
-    Namer.bind(this, tree)
-
-  /**
-   * Bind, then evaluate the given NameTree with this Namer. The result
-   * is translated into a Var[Addr].
-   */
-  def bindAndEval(tree: NameTree[Path]): Var[Addr] = boundNameTreeToAddr(bind(tree))
+    Namer.bind(this.lookup, tree)
 }
 
 private case class FailingNamer(exc: Throwable) extends Namer {
@@ -128,12 +122,21 @@ object Namer  {
     override def toString = "Namer.global"
   }
 
+  /**
+   * Resolve a path to an address set (taking `dtab` into account).
+   */
+  def resolve(dtab: Dtab, path: Path): Var[Addr] =
+    NameInterpreter.bind(dtab, path).map(_.eval).run.flatMap {
+      case Activity.Ok(None) => Var.value(Addr.Neg)
+      case Activity.Ok(Some(names)) => Name.all(names).addr
+      case Activity.Pending => Var.value(Addr.Pending)
+      case Activity.Failed(exc) => Var.value(Addr.Failed(exc))
+    }
+
   /**
    * Resolve a path to an address set (taking [[Dtab.local]] into account).
    */
-  def resolve(path: Path): Var[Addr] = {
-    boundNameTreeToAddr(NameInterpreter.bind(Dtab.base ++ Dtab.local, path))
-  }
+  def resolve(path: Path): Var[Addr] = resolve(Dtab.base ++ Dtab.local, path)
 
   /**
    * Resolve a path to an address set (taking [[Dtab.local]] into account).
@@ -156,18 +159,26 @@ object Namer  {
 
   private val MaxDepth = 100
 
-  private def bind(namer: Namer, tree: NameTree[Path]): Activity[NameTree[Name.Bound]] =
-    bind(namer, 0)(tree map { path => Name.Path(path) })
+  /**
+   * Bind the given tree by recursively following paths and looking them
+   * up with the provided `lookup` function. A recursion depth of up to
+   * 100 is allowed.
+   */
+  def bind(
+    lookup: Path => Activity[NameTree[Name]],
+    tree: NameTree[Path]
+  ): Activity[NameTree[Name.Bound]] =
+    bind(lookup, 0)(tree map { path => Name.Path(path) })
 
   private[this] def bindUnion(
-    namer: Namer,
+    lookup: Path => Activity[NameTree[Name]],
     depth: Int,
     trees: Seq[Weighted[Name]]
   ): Activity[NameTree[Name.Bound]] = {
 
     val weightedTreeVars: Seq[Var[Activity.State[NameTree.Weighted[Name.Bound]]]] = trees.map {
       case Weighted(w, t) =>
-        val treesAct: Activity[NameTree[Name.Bound]] = bind(namer, depth)(t)
+        val treesAct: Activity[NameTree[Name.Bound]] = bind(lookup, depth)(t)
         treesAct.map(Weighted(w, _)).run
     }
 
@@ -193,30 +204,30 @@ object Namer  {
   }
 
   // values of the returned activity are simplified and contain no Alt nodes
-  private def bind(namer: Namer, depth: Int)(tree: NameTree[Name])
+  private def bind(lookup: Path => Activity[NameTree[Name]], depth: Int)(tree: NameTree[Name])
   : Activity[NameTree[Name.Bound]] =
     if (depth > MaxDepth)
       Activity.exception(new IllegalArgumentException("Max recursion level reached."))
     else tree match {
-      case Leaf(Name.Path(path)) => namer.lookup(path) flatMap bind(namer, depth+1)
+      case Leaf(Name.Path(path)) => lookup(path) flatMap bind(lookup, depth+1)
       case Leaf(bound@Name.Bound(_)) => Activity.value(Leaf(bound))
 
       case Fail => Activity.value(Fail)
       case Neg => Activity.value(Neg)
       case Empty => Activity.value(Empty)
 
       case Union() => Activity.value(Neg)
-      case Union(Weighted(_, tree)) => bind(namer, depth)(tree)
-      case Union(trees@_*) => bindUnion(namer, depth, trees)
+      case Union(Weighted(_, tree)) => bind(lookup, depth)(tree)
+      case Union(trees@_*) => bindUnion(lookup, depth, trees)
 
       case Alt() => Activity.value(Neg)
-      case Alt(tree) => bind(namer, depth)(tree)
+      case Alt(tree) => bind(lookup, depth)(tree)
       case Alt(trees@_*) =>
         def loop(trees: Seq[NameTree[Name]]): Activity[NameTree[Name.Bound]] =
           trees match {
             case Nil => Activity.value(Neg)
             case Seq(head, tail@_*) =>
-              bind(namer, depth)(head) flatMap {
+              bind(lookup, depth)(head) flatMap {
                 case Fail => Activity.value(Fail)
                 case Neg => loop(tail)
                 case head => Activity.value(head)

finagle-core/src/main/scala/com/twitter/finagle/client/ClientRegistry.scala

@@ -72,12 +72,13 @@ private[finagle] object RegistryEntryLifecycle {
       next: Stack[ServiceFactory[Req, Rep]]
     ): Stack[ServiceFactory[Req, Rep]] = {
       val BindingFactory.Dest(dest) = params[BindingFactory.Dest]
+      val BindingFactory.BaseDtab(baseDtab) = params[BindingFactory.BaseDtab]
 
       // for the benefit of ClientRegistry.expAllRegisteredClientsResolved
       // which waits for these to become non-Pending
       val va = dest match {
         case Name.Bound(va) => va
-        case Name.Path(path) => Namer.resolve(path)
+        case Name.Path(path) => Namer.resolve(baseDtab(), path)
       }
 
       val shown = Showable.show(dest)

finagle-core/src/main/scala/com/twitter/finagle/naming/DefaultInterpreter.scala

@@ -0,0 +1,24 @@
+package com.twitter.finagle.naming
+
+import com.twitter.finagle._
+import com.twitter.util.Activity
+
+/**
+ * Interpret names against the Dtab, using the default evaluation
+ * strategy. Recursively look up and rewrite paths according to entries
+ * matching the path prefix in the Dtab. If a path does not match any
+ * Dtab entry prefix, the global Namer is invoked to resolve it. This is
+ * how paths starting with `/$/` are bound.
+ */
+object DefaultInterpreter extends NameInterpreter {
+
+  override def bind(dtab: Dtab, path: Path): Activity[NameTree[Name.Bound]] = {
+    def lookup(path: Path): Activity[NameTree[Name]] =
+      dtab.lookup(path) match {
+        case NameTree.Neg => Namer.global.bind(NameTree.Leaf(path))
+        case t => Activity.value(t)
+      }
+
+    Namer.bind(lookup, NameTree.Leaf(path))
+  }
+}

finagle-core/src/main/scala/com/twitter/finagle/naming/NameInterpreter.scala

@@ -17,17 +17,15 @@ trait NameInterpreter {
 
 object NameInterpreter extends NameInterpreter {
 
-  /** The default interpreter. Bind `path` using `dtab` as a namer. */
-  val default = new NameInterpreter {
-    override def bind(dtab: Dtab, path: Path) = dtab.bind(NameTree.Leaf(path))
-  }
-
   /**
    * The global interpreter that resolves all names in Finagle.
    *
    * Can be modified to provide a different mechanism for name resolution.
    */
-  @volatile var global: NameInterpreter = default
+  @volatile var global: NameInterpreter = DefaultInterpreter
+
+  /** Java API for setting the interpreter */
+  def setGlobal(nameInterpreter: NameInterpreter) = { global = nameInterpreter }
 
   override def bind(dtab: Dtab, tree: Path): Activity[NameTree[Name.Bound]] =
     global.bind(dtab, tree)