Skip to content

Commit

Permalink
finatra: Remove commons-lang as a dependency
Browse files Browse the repository at this point in the history
Problem

Apache commons-lang is used in many places where suitable alternatives exist in
the jdk or scala stdlib. It seems silly to carry an external dependency for a
small amount of redundant functions.

Solution/Result

Remove Apache commons-lang as a dependency by replacing uses with jdk or scala
alternatives.

JIRA Issues: CSL-8155

Differential Revision: https://phabricator.twitter.biz/D354013
  • Loading branch information
David Rusek authored and jenkins committed Sep 5, 2019
1 parent d4c84f1 commit 206df64
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 48 deletions.
67 changes: 31 additions & 36 deletions server/src/main/scala/com/twitter/server/Admin.scala
Original file line number Diff line number Diff line change
@@ -1,49 +1,23 @@
package com.twitter.server

import com.twitter.app.App
import com.twitter.finagle.http.Method
import com.twitter.finagle.stats.{
WithHistogramDetails,
AggregateWithHistogramDetails,
DelegatingStatsReceiver,
AggregateWithHistogramDetails
StatsReceiver,
WithHistogramDetails
}
import com.twitter.finagle.http.Method
import com.twitter.server.AdminHttpServer.Route
import com.twitter.server.handler._
import com.twitter.server.view._

object Admin {

/**
* Common constants for [[AdminHttpServer.Route]]'s `group`.
* Defines many of the default `/admin/` HTTP routes.
*/
object Grouping {
val ProcessInfo: String = "Process Info"
val PerfProfile: String = "Performance Profile"
val Utilities: String = "Utilities"
val Metrics: String = "Metrics"
}

/**
* Constants for Admin endpoints.
*/
object Path {
val Root: String = ""
val Admin: String = "/admin"
val Clients: String = Admin + "/clients/"
val Servers: String = Admin + "/servers/"
val Files: String = Admin + "/files/"
}
}

/**
* Defines many of the default `/admin/` HTTP routes.
*/
trait Admin { self: App with AdminHttpServer with Stats =>
import Admin._
import AdminHttpServer.Route
import Admin.Grouping

override protected def routes: Seq[Route] = {

def adminRoutes(statsReceiver: StatsReceiver, app: App): Seq[Route] = {
// we handle ping in-band with the global default worker pool
val colocatedRoutes = Seq(
Route(
Expand Down Expand Up @@ -82,7 +56,7 @@ trait Admin { self: App with AdminHttpServer with Stats =>
),
Route(
path = "/admin/server_info",
handler = new TextBlockView().andThen(new ServerInfoHandler(self)),
handler = new TextBlockView().andThen(new ServerInfoHandler()),
alias = "Build Properties",
group = Some(Grouping.ProcessInfo),
includeInIndex = true
Expand Down Expand Up @@ -166,7 +140,7 @@ trait Admin { self: App with AdminHttpServer with Stats =>
),
Route(
path = "/admin/shutdown",
handler = new ShutdownHandler(this),
handler = new ShutdownHandler(app),
alias = "Shutdown",
group = Some(Grouping.Utilities),
includeInIndex = true,
Expand Down Expand Up @@ -264,7 +238,7 @@ trait Admin { self: App with AdminHttpServer with Stats =>
group = Some(Grouping.Utilities),
includeInIndex = true
)
).map(Route.isolate)
).map(AdminHttpServer.Route.isolate)

// If histograms are available, add an additional endpoint
val histos = DelegatingStatsReceiver
Expand Down Expand Up @@ -306,4 +280,25 @@ trait Admin { self: App with AdminHttpServer with Stats =>
)
}
}

/**
* Common constants for [[AdminHttpServer.Route]]'s `group`.
*/
object Grouping {
val ProcessInfo: String = "Process Info"
val PerfProfile: String = "Performance Profile"
val Utilities: String = "Utilities"
val Metrics: String = "Metrics"
}

/**
* Constants for Admin endpoints.
*/
object Path {
val Root: String = ""
val Admin: String = "/admin"
val Clients: String = Admin + "/clients/"
val Servers: String = Admin + "/servers/"
val Files: String = Admin + "/files/"
}
}
10 changes: 5 additions & 5 deletions server/src/main/scala/com/twitter/server/AdminHttpServer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ object AdminHttpServer {

}

trait AdminHttpServer { self: App =>
trait AdminHttpServer { self: App with Stats =>
import AdminHttpServer._

// We use slf4-api directly b/c we're in a trait and want the trait class to be the Logger name
Expand Down Expand Up @@ -159,7 +159,8 @@ trait AdminHttpServer { self: App =>
LoadService[AdminHttpMuxHandler]()
.map(handler => Route.from(handler.route))
.map(Route.isolate)
private var allRoutes: Seq[Route] = loadServiceRoutes

private[this] var allRoutes: Seq[Route] = loadServiceRoutes

/**
* The address to which the Admin HTTP server is bound.
Expand All @@ -179,8 +180,7 @@ trait AdminHttpServer { self: App =>
addAdminRoutes(Seq(route))
}

@deprecated("Routes should be added via `AdminHttpServer#addAdminRoutes`", "2018-10-17")
protected def routes: Seq[Route] = Nil
def routes: Seq[Route] = allRoutes

/**
* Name used for registration in the [[com.twitter.util.registry.Library]]
Expand Down Expand Up @@ -312,7 +312,7 @@ trait AdminHttpServer { self: App =>
}

premain {
addAdminRoutes(routes)
addAdminRoutes(Admin.adminRoutes(statsReceiver, self))
startServer()
}
}
5 changes: 2 additions & 3 deletions server/src/main/scala/com/twitter/server/TwitterServer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,12 @@ trait TwitterServer
extends App
with Slf4jBridge
with Logging
with Stats
with Linters
with DtabFlags
with Hooks
with AdminHttpServer
with Admin
with Lifecycle
with Stats {
with Lifecycle {

/** Don't let applications opt-out */
final override val suppressGracefulShutdownErrors: Boolean = false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import scala.collection.JavaConverters._
* A simple http service for serving up information pulled from a build.properties
* file.
*/
class ServerInfoHandler(obj: AnyRef) extends Service[Request, Response] {
class ServerInfoHandler() extends Service[Request, Response] {
private[this] val mxRuntime = ManagementFactory.getRuntimeMXBean

private[this] val registry = GlobalRegistry.get
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class ServerInfoHandlerTest extends FunSuite {
GlobalRegistry.get.exists(_.key.startsWith(key))

test("ServerInfo handler display server information") {
val handler = new ServerInfoHandler(this)
val handler = new ServerInfoHandler()
val req = Request("/")
val res = Await.result(handler(req))

Expand All @@ -31,7 +31,7 @@ class ServerInfoHandlerTest extends FunSuite {
}

test("ServerInfo handler returns the right content-type") {
val handler = new ServerInfoHandler(this)
val handler = new ServerInfoHandler()
val req = Request("/")
val res = Await.result(handler(req))
assert(res.contentType.contains("application/json;charset=UTF-8"))
Expand All @@ -42,7 +42,7 @@ class ServerInfoHandlerTest extends FunSuite {
test(s"ServerInfo handler adds ${key.mkString(" ")} to Global Registry on instantiation") {
GlobalRegistry.withRegistry(new SimpleRegistry) {
assert(!isRegistered(key))
new ServerInfoHandler(this)
new ServerInfoHandler()
assert(isRegistered(key))
}
}
Expand Down

3 comments on commit 206df64

@krrrr38
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgodave before this commit, we can add admin route by overriding def routes. currently overriding def routes has no meanings, it's breaking changes. Now we should use addAdminRoute, so propose that removing def routes.

image

@yufangong
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krrrr38 Thanks! Can you open a GitHub issue for this?

@krrrr38
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yufangong yes, please. #61

Please sign in to comment.