finatra/inject: Remove FilteredThriftClientModule & friends

Problem/Solution FilteredThriftClientModule is deprecated for quite a while, let's remove it and related interfaces and tests! Differential Revision: https://phabricator.twitter.biz/D687663
twitter · Jun 17, 2021 · 008d8ca · 008d8ca
1 parent c515920
commit 008d8ca
Show file tree

Hide file tree

Showing 18 changed files with 57 additions and 1,287 deletions.
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
@@ -10,6 +10,11 @@ Unreleased
 Changed
 ~~~~~~~
 
+* inject-thrift-client (BREAKING API CHANGE): Removed the deprecated
+  `c.t.inject.thrift.modules.FilteredThriftClientModule`. Please use its successor
+  `c.t.inject.thrift.modules.ThriftMethodBuilderClientModule` for per-method configuration of a
+  Thrift client. ``PHAB_ID=D687663``
+
 * thrift: Add `service_class` to Finatra library thrift registry entry. ``PHAB_ID=D687117``
 
 * finatra (BREAKING API CHANGE): Update to use the new util/util-jackson `ScalaObjectMapper` for

diff --git a/...mapper/src/test/scala/com/twitter/finatra/multiserver/Add1HttpServer/Add1Controller.scala b/...mapper/src/test/scala/com/twitter/finatra/multiserver/Add1HttpServer/Add1Controller.scala
@@ -3,10 +3,9 @@ package com.twitter.finatra.multiserver.Add1HttpServer
 import com.twitter.adder.thriftscala.Adder
 import com.twitter.finagle.http.Request
 import com.twitter.finatra.http.Controller
-import com.twitter.util.Future
 import javax.inject.Inject
 
-class Add1Controller @Inject() (adder: Adder[Future]) extends Controller {
+class Add1Controller @Inject() (adder: Adder.MethodPerEndpoint) extends Controller {
 
   get("/add1") { request: Request =>
     val num = request.getIntParam("num")

diff --git a/...c/test/scala/com/twitter/finatra/multiserver/Add1HttpServer/AdderThriftClientModule.scala b/...c/test/scala/com/twitter/finatra/multiserver/Add1HttpServer/AdderThriftClientModule.scala
@@ -4,71 +4,58 @@ import com.twitter.adder.thriftscala.Adder
 import com.twitter.adder.thriftscala.Adder._
 import com.twitter.conversions.DurationOps._
 import com.twitter.finagle.Filter
-import com.twitter.inject.thrift.filters.ThriftClientFilterBuilder
-import com.twitter.inject.thrift.modules.FilteredThriftClientModule
-import com.twitter.util.Future
+import com.twitter.inject.Injector
+import com.twitter.inject.exceptions.PossiblyRetryable
+import com.twitter.inject.thrift.ThriftMethodBuilderFactory
+import com.twitter.inject.thrift.modules.ThriftMethodBuilderClientModule
 
 object AdderThriftClientModule
-    extends FilteredThriftClientModule[Adder[Future], Adder.ServiceIface] {
+    extends ThriftMethodBuilderClientModule[Adder.ServicePerEndpoint, Adder.MethodPerEndpoint] {
 
   override val label = "adder-thrift"
   override val dest = "flag!adder-thrift-server"
 
-  override def filterServiceIface(
-    serviceIface: ServiceIface,
-    filter: ThriftClientFilterBuilder
-  ): ServiceIface = {
+  override def configureServicePerEndpoint(
+    injector: Injector,
+    builder: ThriftMethodBuilderFactory[Adder.ServicePerEndpoint],
+    servicePerEndpoint: Adder.ServicePerEndpoint
+  ): Adder.ServicePerEndpoint = {
 
-    serviceIface.copy(
-      add1 = filter
-        .method(Add1)
-        .withExceptionFilter(
-          Filter.identity[Add1.Args, Add1.SuccessType]
-        ) // Example of replacing the default exception filter
-        .withTimeout(3.minutes)
-        .withExponentialRetry(
-          shouldRetryResponse = PossiblyRetryableExceptions,
-          start = 50.millis,
-          multiplier = 2,
-          retries = 3
-        )
-        .withRequestTimeout(1.minute)
-        .andThen(serviceIface.add1),
-      add1String = filter
-        .method(Add1String)
-        .withTimeout(3.minutes)
-        .withExponentialRetry(
-          shouldRetryResponse = PossiblyRetryableExceptions,
-          start = 50.millis,
-          multiplier = 2,
-          retries = 3
-        )
-        .withRequestTimeout(1.minute)
-        .andThen(serviceIface.add1String),
-      add1Slowly = filter
-        .method(Add1Slowly)
-        .withTimeout(3.minutes)
-        .withExponentialRetry(
-          shouldRetryResponse = PossiblyRetryableExceptions,
-          start = 50.millis,
-          multiplier = 2,
-          retries = 3
-        )
-        .withRequestTimeout(
-          1.millis
-        ) // We purposely set a very small timeout so that we can test handling IndividualRequestTimeoutException
-        .andThen(serviceIface.add1Slowly),
-      add1AlwaysError = filter
-        .method(Add1AlwaysError)
-        .withTimeout(3.minutes)
-        .withExponentialRetry(
-          shouldRetryResponse = PossiblyRetryableExceptions,
-          start = 50.millis,
-          multiplier = 2,
-          retries = 3
-        )
-        .withRequestTimeout(1.minute)
-        .andThen(serviceIface.add1AlwaysError)
-    )
+    servicePerEndpoint
+      .withAdd1(
+        builder
+          .method(Add1)
+          .withExceptionFilter(Filter.identity[Add1.Args, Add1.SuccessType])
+          .withTimeoutTotal(3.minutes)
+          .withTimeoutPerRequest(1.minute)
+          .withRetryForClassifier(PossiblyRetryable.ResponseClassifier)
+          .withMaxRetries(3)
+          .service)
+      .withAdd1String(
+        builder
+          .method(Add1String)
+          .withTimeoutTotal(3.minutes)
+          .withTimeoutPerRequest(1.minute)
+          .withRetryForClassifier(PossiblyRetryable.ResponseClassifier)
+          .withMaxRetries(3)
+          .service
+      ).withAdd1Slowly(
+        builder
+          .method(Add1Slowly)
+          .withTimeoutTotal(3.minutes)
+          // We purposely set a very small timeout so that we can test handling IndividualRequestTimeoutException
+          .withTimeoutPerRequest(1.millis)
+          .withRetryForClassifier(PossiblyRetryable.ResponseClassifier)
+          .withMaxRetries(3)
+          .service
+      ).withAdd1AlwaysError(
+        builder
+          .method(Add1AlwaysError)
+          .withTimeoutTotal(3.minutes)
+          .withTimeoutPerRequest(1.minute)
+          .withRetryForClassifier(PossiblyRetryable.ResponseClassifier)
+          .withMaxRetries(3)
+          .service
+      )
   }
 }
diff --git a/...tp-mapper/src/test/scala/com/twitter/finatra/multiserver/test/Add1ServerFeatureTest.scala b/...tp-mapper/src/test/scala/com/twitter/finatra/multiserver/test/Add1ServerFeatureTest.scala
@@ -10,14 +10,14 @@ import com.twitter.util.mock.Mockito
 
 class Add1ServerFeatureTest extends FeatureTest with Mockito {
 
-  val adderFuture = mock[Adder[Future]]
+  val adderMpe = mock[Adder.MethodPerEndpoint]
 
   override val server =
     new EmbeddedHttpServer(new Add1Server)
-      .bind[Adder[Future]].toInstance(adderFuture)
+      .bind[Adder.MethodPerEndpoint].toInstance(adderMpe)
 
   test("add1") {
-    adderFuture.add1(5) returns Future(6)
+    adderMpe.add1(5) returns Future(6)
 
     server.httpGet("/add1?num=5", andExpect = Status.Ok, withBody = "6")
   }

diff --git a/...p-mapper/src/test/scala/com/twitter/finatra/multiserver/test/MultiServerFeatureTest.scala b/...p-mapper/src/test/scala/com/twitter/finatra/multiserver/test/MultiServerFeatureTest.scala
@@ -31,8 +31,6 @@ class MultiServerFeatureTest extends Test with HttpTest with ThriftTest {
 
     add1HttpServer.inMemoryStats.counters.assert("clnt/adder-thrift/Adder/add1/requests", 4)
     add1HttpServer.inMemoryStats.counters.assert("clnt/adder-thrift/Adder/add1/success", 2)
-    // invocations --> incremented every time we call/invoke the method.
-    add1HttpServer.inMemoryStats.counters.assert("clnt/adder-thrift/Adder/add1/invocations", 2)
     add1HttpServer.inMemoryStats.counters.assert("clnt/adder-thrift/Adder/add1/failures", 2)
     add1HttpServer.inMemoryStats.counters.assert(
       "clnt/adder-thrift/Adder/add1/failures/org.apache.thrift.TApplicationException",
@@ -49,9 +47,6 @@ class MultiServerFeatureTest extends Test with HttpTest with ThriftTest {
     // client stats
     add1HttpServer.inMemoryStats.counters.assert("clnt/adder-thrift/Adder/add1String/requests", 6)
     add1HttpServer.inMemoryStats.counters.assert("clnt/adder-thrift/Adder/add1String/success", 1)
-    // invocations --> incremented every time we call/invoke the method.
-    add1HttpServer.inMemoryStats.counters
-      .assert("clnt/adder-thrift/Adder/add1String/invocations", 2)
     add1HttpServer.inMemoryStats.counters.assert("clnt/adder-thrift/Adder/add1String/failures", 5)
     add1HttpServer.inMemoryStats.counters.assert(
       "clnt/adder-thrift/Adder/add1String/failures/org.apache.thrift.TApplicationException",
@@ -93,11 +88,6 @@ class MultiServerFeatureTest extends Test with HttpTest with ThriftTest {
     )
     add1HttpServer.inMemoryStats.counters
       .get("clnt/adder-thrift/Adder/add1AlwaysError/success") should be(None)
-    // invocations --> incremented every time we call/invoke the method.
-    add1HttpServer.inMemoryStats.counters.assert(
-      "clnt/adder-thrift/Adder/add1AlwaysError/invocations",
-      numHttpRequests
-    )
     add1HttpServer.inMemoryStats.counters.assert(
       "clnt/adder-thrift/Adder/add1AlwaysError/failures",
       expectedFailedThriftRequests
@@ -146,14 +136,11 @@ class MultiServerFeatureTest extends Test with HttpTest with ThriftTest {
     ) // our requests should total at most the expected but may be less.
     add1HttpServer.inMemoryStats.counters
       .get("clnt/adder-thrift/Adder/add1Slowly/success") should be(None)
-    // invocations --> incremented every time we call/invoke the method.
-    add1HttpServer.inMemoryStats.counters
-      .assert("clnt/adder-thrift/Adder/add1Slowly/invocations", numHttpRequests)
     add1HttpServer.inMemoryStats.counters.assert("clnt/adder-thrift/Adder/add1Slowly/failures")(
       _ <= expectedFailedThriftRequests
     ) // our failures should total at most the expected but may be less.
     add1HttpServer.inMemoryStats.counters.assert(
-      "clnt/adder-thrift/Adder/add1Slowly/failures/com.twitter.util.TimeoutException"
+      "clnt/adder-thrift/add1Slowly/logical/failures/com.twitter.finagle.IndividualRequestTimeoutException"
     )(
       _ <= expectedFailedThriftRequests
     ) // our failures should total at most the expected but may be less.
@@ -164,7 +151,7 @@ class MultiServerFeatureTest extends Test with HttpTest with ThriftTest {
       _ <= expectedFailedThriftRequests
     ) // our requests should total at most the expected but may be less.
     add1HttpServer.inMemoryStats.counters
-      .assert("clnt/adder-thrift/failures/com.twitter.util.TimeoutException")(
+      .assert("clnt/adder-thrift/failures/com.twitter.finagle.IndividualRequestTimeoutException")(
         _ <= expectedFailedThriftRequests
       ) // our requests should total at most the expected but may be less.
 

diff --git a/...t-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterBuilder.scala b/...t-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterBuilder.scala