Skip to content

Commit

Permalink
finagle-toggle: Make StandardToggleMap mutable
Browse files Browse the repository at this point in the history
Problem

StandardToggleMap has an underlying mutable map, but it's difficult to access.

Solution

Make StandardToggleMap a `ToggleMap.Mutable` that proxies the mutating method
to the underlying mutable map.

JIRA Issues: CSL-6348

Differential Revision: https://phabricator.twitter.biz/D167046
  • Loading branch information
mosesn authored and jenkins committed May 5, 2018
1 parent 63eb5e1 commit 2a4fbfb
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 50 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ private[handler] object ToggleHandler {
* For create and update, and an additional "fraction" request parameter
* must be set as well.
*/
class ToggleHandler private[handler] (registeredLibrariesFn: () => Map[String, ToggleMap])
class ToggleHandler private[handler] (registeredLibrariesFn: () => Map[String, ToggleMap.Mutable])
extends Service[Request, Response] {

import ToggleHandler._
Expand Down Expand Up @@ -232,11 +232,6 @@ class ToggleHandler private[handler] (registeredLibrariesFn: () => Map[String, T
}
}

private[this] def findMutable(toggleMap: ToggleMap): Option[ToggleMap.Mutable] =
ToggleMap.components(toggleMap).collectFirst {
case mut: ToggleMap.Mutable => mut
}

/**
* Returns any error messages, or an empty `Seq` if successful.
*
Expand All @@ -259,13 +254,8 @@ class ToggleHandler private[handler] (registeredLibrariesFn: () => Map[String, T
val fraction = f.toDouble
if (Toggle.isValidFraction(fraction)) {
val toggleMap = registeredLibrariesFn()(libraryName)
findMutable(toggleMap) match {
case None =>
errors += s"Mutable ToggleMap not found for '$libraryName'"
case Some(mut) =>
log.info(s"Set $libraryName's toggle $id to $fraction")
mut.put(id, fraction)
}
log.info(s"Set $libraryName's toggle $id to $fraction")
toggleMap.put(id, fraction)
} else {
errors += s"Fraction must be [0.0-1.0], was: '$fractionStr'"
}
Expand All @@ -291,13 +281,8 @@ class ToggleHandler private[handler] (registeredLibrariesFn: () => Map[String, T
): Seq[String] = {
val errors = new ArrayBuffer[String]()
val toggleMap = registeredLibrariesFn()(libraryName)
findMutable(toggleMap) match {
case None =>
errors += s"Mutable ToggleMap not found for '$libraryName'"
case Some(mut) =>
log.info(s"Deleted $libraryName's toggle $id")
mut.remove(id)
}
log.info(s"Deleted $libraryName's toggle $id")
toggleMap.remove(id)
errors
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,12 @@
package com.twitter.server.handler

import com.twitter.finagle.stats.NullStatsReceiver
import com.twitter.finagle.toggle.{NullToggleMap, Toggle, ToggleMap}
import com.twitter.finagle.toggle.{Toggle, ToggleMap}
import com.twitter.server.handler.ToggleHandler._
import org.junit.runner.RunWith
import org.scalatest.FunSuite
import org.scalatest.junit.JUnitRunner
import scala.collection.immutable
import scala.collection.mutable.ArrayBuffer

@RunWith(classOf[JUnitRunner])
class ToggleHandlerTest extends FunSuite {

test("renders empty registeredLibraries") {
Expand All @@ -24,8 +21,8 @@ class ToggleHandlerTest extends FunSuite {

test("toLibraryToggles for empty ToggleMaps") {
val mappings = Map(
"com.twitter.Empty1" -> new ToggleMap.Immutable(immutable.Seq.empty),
"com.twitter.Empty2" -> new ToggleMap.Immutable(immutable.Seq.empty)
"com.twitter.Empty1" -> ToggleMap.newMutable(),
"com.twitter.Empty2" -> ToggleMap.newMutable()
)
val libs = new ToggleHandler(() => mappings)
.toLibraries(ParsedPath(None, None))
Expand All @@ -50,7 +47,12 @@ class ToggleHandlerTest extends FunSuite {

val tm0 = new ToggleMap.Immutable(immutable.Seq(t0, t1))
val tm1 = new ToggleMap.Immutable(immutable.Seq(t0b, t2))
val mappings = Map("com.twitter.With" -> tm0.orElse(tm1))
val immutableMap = tm0.orElse(tm1)
val mappings = Map("com.twitter.With" -> new ToggleMap.Mutable with ToggleMap.Proxy {
def underlying: ToggleMap = immutableMap
def put(id: String, fraction: Double): Unit = ???
def remove(id: String): Unit = ???
})
val libs = new ToggleHandler(() => mappings)
.toLibraries(ParsedPath(None, None))

Expand Down Expand Up @@ -133,7 +135,11 @@ class ToggleHandlerTest extends FunSuite {
tm("com.twitter.map0toggle0")(5)
tm("com.twitter.map0toggle1")(5)

val mappings = Map("com.twitter.map0" -> tm)
val mappings = Map("com.twitter.map0" -> new ToggleMap.Mutable with ToggleMap.Proxy {
def underlying: ToggleMap = tm
def put(id: String, fraction: Double): Unit = ???
def remove(id: String): Unit = ???
})
val handler = new ToggleHandler(() => mappings)

def currentForId(id: String): ToggleHandler.Current = {
Expand Down Expand Up @@ -172,15 +178,6 @@ class ToggleHandlerTest extends FunSuite {
assert(!tog(30))
}

test("setToggle missing Mutable ToggleMap") {
val libName = "some.other.lib"
val handler = new ToggleHandler(() => Map(libName -> NullToggleMap))

val errors = handler.setToggle(libName, "com.handler.T0", Some("0.0"))
assert(errors.nonEmpty)
assert(errors.contains("Mutable ToggleMap not found for 'some.other.lib'"))
}

test("setToggle missing fraction") {
val libName = "com.handler.Mutable"
val handler = new ToggleHandler(() => Map(libName -> ToggleMap.newMutable()))
Expand Down Expand Up @@ -225,22 +222,13 @@ class ToggleHandlerTest extends FunSuite {
assert(!tog.isDefinedAt(30))
}

test("deleteToggle missing Mutable ToggleMap") {
val libName = "some.other.lib"
val handler = new ToggleHandler(() => Map(libName -> NullToggleMap))

val errors = handler.deleteToggle(libName, "com.handler.T0")
assert(errors.nonEmpty)
assert(errors.contains("Mutable ToggleMap not found for 'some.other.lib'"))
}

test("parsePath") {
def assertParsePathOk(
path: String,
expectedLibraryName: Option[String],
expectedId: Option[String]
): Unit = {
val handler = new ToggleHandler(() => Map("com.twitter.lib" -> NullToggleMap))
val handler = new ToggleHandler(() => Map("com.twitter.lib" -> ToggleMap.newMutable()))
val errors = new ArrayBuffer[String]()
val parsed = handler.parsePath(path, errors)
assert(errors.isEmpty, errors.mkString(", "))
Expand All @@ -259,7 +247,7 @@ class ToggleHandlerTest extends FunSuite {
}

test("parsePath invalid path format") {
val handler = new ToggleHandler(() => Map("com.twitter.lib" -> NullToggleMap))
val handler = new ToggleHandler(() => Map("com.twitter.lib" -> ToggleMap.newMutable()))
def assertPathFormat(path: String): Unit = {
val errors = new ArrayBuffer[String]()
handler.parsePath(path, errors)
Expand All @@ -272,14 +260,14 @@ class ToggleHandlerTest extends FunSuite {
}

test("parsePath unknown libraryName") {
val handler = new ToggleHandler(() => Map("com.twitter.lib" -> NullToggleMap))
val handler = new ToggleHandler(() => Map("com.twitter.lib" -> ToggleMap.newMutable()))
val errors = new ArrayBuffer[String]()
handler.parsePath("/admin/toggles/com.twitter.erm", errors)
assert(errors.contains("Unknown library name: 'com.twitter.erm'"))
}

test("parsePath invalid id") {
val handler = new ToggleHandler(() => Map("com.twitter.lib" -> NullToggleMap))
val handler = new ToggleHandler(() => Map("com.twitter.lib" -> ToggleMap.newMutable()))
val errors = new ArrayBuffer[String]()
handler.parsePath("/admin/toggles/com.twitter.lib/FLEEK", errors)
assert(errors.contains("Invalid id: 'FLEEK'"))
Expand Down

0 comments on commit 2a4fbfb

Please sign in to comment.