From 29e9805f4520b5c832fa824c53f0d6b7fd40c0fa Mon Sep 17 00:00:00 2001 From: Christopher Coco Date: Wed, 17 Jan 2018 23:46:37 +0000 Subject: [PATCH 01/12] Twitter-oss: Update OSS libraries post release to 18.2.0-SNAPSHOT Summary: Problem We want to update the next version of our Twitter OSS libraries 18.2.0-SNAPSHOT - util - scrooge - finagle - twitter-server - finatra Solution Update the libraries post release. JIRA Issues: CSL-5786 Differential Revision: https://phabricator.twitter.biz/D128600 --- README.md | 4 ++-- build.sbt | 2 +- project/plugins.sbt | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 8c332dd17d..ed0c0b30f4 100644 --- a/README.md +++ b/README.md @@ -1,7 +1,7 @@ # Finatra -[![Build Status](https://secure.travis-ci.org/twitter/finatra.png?branch=master)](https://travis-ci.org/twitter/finatra?branch=master) -[![Test Coverage](https://codecov.io/github/twitter/finatra/coverage.svg?branch=master)](https://codecov.io/github/twitter/finatra?branch=master) +[![Build Status](https://secure.travis-ci.org/twitter/finatra.png?branch=develop)](https://travis-ci.org/twitter/finatra?branch=develop) +[![Test Coverage](https://codecov.io/github/twitter/finatra/coverage.svg?branch=develop)](https://codecov.io/github/twitter/finatra?branch=develop) [![Project status](https://img.shields.io/badge/status-active-brightgreen.svg)](#status) [![Maven Central](https://maven-badges.herokuapp.com/maven-central/com.twitter/finatra-http_2.12/badge.svg)][maven-central] [![Gitter](https://badges.gitter.im/Join%20Chat.svg)](https://gitter.im/twitter/finatra) diff --git a/build.sbt b/build.sbt index 14152dc605..b6bffe4179 100644 --- a/build.sbt +++ b/build.sbt @@ -4,7 +4,7 @@ import scoverage.ScoverageKeys concurrentRestrictions in Global += Tags.limit(Tags.Test, 1) // All Twitter library releases are date versioned as YY.MM.patch -val releaseVersion = "18.1.0" +val releaseVersion = "18.2.0-SNAPSHOT" lazy val buildSettings = Seq( version := releaseVersion, diff --git a/project/plugins.sbt b/project/plugins.sbt index fdd5415108..f8ee3bbb00 100644 --- a/project/plugins.sbt +++ b/project/plugins.sbt @@ -3,7 +3,7 @@ resolvers ++= Seq( Resolver.sonatypeRepo("snapshots") ) -val releaseVersion = "18.1.0" +val releaseVersion = "18.2.0-SNAPSHOT" addSbtPlugin("com.twitter" % "scrooge-sbt-plugin" % releaseVersion) From 42ef3bcc1ad8fcda877245554ceb4c3ffcd348c8 Mon Sep 17 00:00:00 2001 From: Christopher Coco Date: Sat, 20 Jan 2018 05:16:06 +0000 Subject: [PATCH 02/12] inject-thrift-client: Introduce the `c.t.inject.thrift.modules.ServicePerEndpointModule` Summary: Problem We would like a way to be able to create Finagle Thrift clients in Finatra using the `MethodBuilder` per-method configuration semantics and making use of the new Scrooge `ServicePerEndpoint` and `MethodPerEndpoint` implementations. Solution Introduce the `c.t.inject.thrift.modules.ServicePerEndpointModule` which allows users to configure a Scrooge-generated `ServicePerEndpoint` using MethodBuilder semantics to apply timeout, retry, and response classification semantics along with the ability to specify arbitrary user-defined filters. Note: This module only builds a client from the ThriftMux protocol. The `c.t.inject.thrift.modules.ServicePerEndpointModule` can be parameterized to support creating either a `ServicePerEndpoint` or `RepRepServicePerEndpoint` interface to the Thrift client. Result When a user implements a `ServicePerEndpointModule` they will be able to inject either the "service-per-endpoint" client interface or the `MethodPerEndpoint` client interface into classes in their server. Client semantics can be specified on the client or per-method and filters can be applied over the entire "service-per-endpoint" or per-method. JIRA Issues: CSL-5783 Differential Revision: https://phabricator.twitter.biz/D128196 --- CHANGELOG.md | 3 + README.md | 2 +- build.sbt | 2 +- doc/README.md | 4 +- doc/src/sphinx/user-guide/http/clients.rst | 6 + doc/src/sphinx/user-guide/index.rst | 6 + doc/src/sphinx/user-guide/thrift/clients.rst | 270 ++++++++++++++++++ .../inject/thrift/ThriftMethodBuilder.scala | 253 ++++++++++++++++ .../filters/ThriftClientFilterBuilder.scala | 1 + .../filters/ThriftClientFilterChain.scala | 1 + .../internal/DefaultAndThenServiceImpl.scala | 1 - .../filters/ThriftClientExceptionFilter.scala | 4 +- .../FilteredThriftClientFlagsModule.scala | 1 + .../modules/FilteredThriftClientModule.scala | 1 + .../modules/ServicePerEndpointModule.scala | 231 +++++++++++++++ .../thrift/modules/ThriftClientModule.scala | 2 +- ...ilteredThriftClientModuleFeatureTest.scala | 60 ++-- ...pServicePerEndpointModuleFeatureTest.scala | 117 ++++++++ ...gServicePerEndpointModuleFeatureTest.scala | 163 +++++++++++ ...rythingThriftClientModuleFeatureTest.scala | 186 ++++++++++++ .../thrift/EchoHttpServerFeatureTest.scala | 45 --- .../thrift/GreeterHttpServerFeatureTest.scala | 39 --- ...eServicePerEndpointModuleFeatureTest.scala | 78 +++++ .../http_server/GreeterHttpServer.scala | 16 -- .../integration/AbstractThriftService.scala | 10 + .../thrift/integration/TestHttpServer.scala | 24 ++ .../TestThriftServer.scala} | 14 +- .../filtered}/ByeThriftClientFilter.scala | 6 +- .../GreeterFilteredThriftClientModule.scala | 9 +- .../filtered}/GreeterHttpController.scala | 2 +- .../filtered/GreeterThriftService.scala} | 6 +- .../filtered}/HelloThriftClientFilter.scala | 2 +- .../filtered}/HiThriftClientFilter.scala | 2 +- .../filtered}/ThriftClientFilter.scala | 2 +- .../filters/HiLoggingTypeAgnosticFilter.scala | 23 ++ .../MethodLoggingTypeAgnosticFilter.scala} | 4 +- .../SetTimesEchoTypeAgnosticFilter.scala | 23 ++ .../http_server/EchoHttpController.scala | 20 -- .../http_server/EchoHttpServer.scala | 16 -- .../http_server/EchoThriftClientModule.scala | 10 - .../integration/inheritance/EchoFilter.scala | 22 ++ .../integration/inheritance/PingFilter.scala | 22 ++ .../inheritance/ServiceBHttpController.scala | 17 ++ .../inheritance/ServiceBHttpServer.scala | 18 ++ .../ServiceBServicePerEndpointModule.scala | 35 +++ .../inheritance/ServiceBThriftService.scala | 27 ++ .../reqrepserviceperendpoint/ByeFilter.scala | 25 ++ .../ByeHeadersFilter.scala | 25 ++ ...reeterReqRepServicePerEndpointModule.scala | 71 +++++ .../GreeterThriftService.scala | 79 +++++ .../HelloFilter.scala | 27 ++ .../HelloHeadersFilter.scala | 25 ++ .../HiHeadersFilter.scala | 23 ++ ...qRepServicePerEndpointHttpController.scala | 30 ++ .../serviceperendpoint/ByeFilter.scala | 24 ++ .../serviceperendpoint/EchoFilter.scala | 21 ++ .../EchoServicePerEndpointModule.scala | 38 +++ .../EchoThriftService.scala | 33 +++ .../GreeterServicePerEndpointModule.scala | 57 ++++ .../GreeterThriftService.scala | 65 +++++ .../serviceperendpoint/HelloFilter.scala | 24 ++ .../ServicePerEndpointHttpController.scala | 37 +++ .../thrift_server/EchoThriftServer.scala | 38 --- .../thrift_server/MyEchoService.scala | 35 --- .../src/test/thrift/serviceA.thrift | 7 + .../src/test/thrift/serviceB.thrift | 9 + 66 files changed, 2220 insertions(+), 279 deletions(-) create mode 100644 doc/src/sphinx/user-guide/http/clients.rst create mode 100644 doc/src/sphinx/user-guide/thrift/clients.rst create mode 100644 inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/ThriftMethodBuilder.scala create mode 100644 inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ServicePerEndpointModule.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingReqRepServicePerEndpointModuleFeatureTest.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingServicePerEndpointModuleFeatureTest.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingThriftClientModuleFeatureTest.scala delete mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/EchoHttpServerFeatureTest.scala delete mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/GreeterHttpServerFeatureTest.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/InheritanceServicePerEndpointModuleFeatureTest.scala delete mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/GreeterHttpServer.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/AbstractThriftService.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/TestHttpServer.scala rename inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/{filtered_integration/thrift_server/GreeterThriftServer.scala => integration/TestThriftServer.scala} (79%) rename inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/{filtered_integration/http_server => integration/filtered}/ByeThriftClientFilter.scala (57%) rename inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/{filtered_integration/http_server => integration/filtered}/GreeterFilteredThriftClientModule.scala (86%) rename inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/{filtered_integration/http_server => integration/filtered}/GreeterHttpController.scala (88%) rename inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/{filtered_integration/thrift_server/GreeterImpl.scala => integration/filtered/GreeterThriftService.scala} (88%) rename inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/{filtered_integration/http_server => integration/filtered}/HelloThriftClientFilter.scala (87%) rename inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/{filtered_integration/http_server => integration/filtered}/HiThriftClientFilter.scala (86%) rename inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/{filtered_integration/http_server => integration/filtered}/ThriftClientFilter.scala (70%) create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filters/HiLoggingTypeAgnosticFilter.scala rename inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/{filtered_integration/http_server/LogMethodTypeAgnosticFilter.scala => integration/filters/MethodLoggingTypeAgnosticFilter.scala} (71%) create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filters/SetTimesEchoTypeAgnosticFilter.scala delete mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/http_server/EchoHttpController.scala delete mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/http_server/EchoHttpServer.scala delete mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/http_server/EchoThriftClientModule.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/EchoFilter.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/PingFilter.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBHttpController.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBHttpServer.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBServicePerEndpointModule.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBThriftService.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/ByeFilter.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/ByeHeadersFilter.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepServicePerEndpointModule.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterThriftService.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/HelloFilter.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/HelloHeadersFilter.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/HiHeadersFilter.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/ReqRepServicePerEndpointHttpController.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/ByeFilter.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoFilter.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoServicePerEndpointModule.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoThriftService.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterServicePerEndpointModule.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterThriftService.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/HelloFilter.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/ServicePerEndpointHttpController.scala delete mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/thrift_server/EchoThriftServer.scala delete mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/thrift_server/MyEchoService.scala create mode 100644 inject/inject-thrift-client/src/test/thrift/serviceA.thrift create mode 100644 inject/inject-thrift-client/src/test/thrift/serviceB.thrift diff --git a/CHANGELOG.md b/CHANGELOG.md index d57b93d2f2..1045e631fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ All notable changes to this project will be documented in this file. Note that ` ### Added +* inject-thrift-client: Add `c.t.inject.thrift.modules.ServicePerEndpointModule` for + building ThriftMux clients using the `thriftmux.MethodBuilder`. ``PHAB_ID=D128196`` + ### Changed ### Fixed diff --git a/README.md b/README.md index ed0c0b30f4..ff0a90b45f 100644 --- a/README.md +++ b/README.md @@ -132,7 +132,7 @@ Follow [@finatra](https://twitter.com/finatra) on Twitter for updates. ## License -Copyright 2013-2017 Twitter, Inc. +Copyright 2013-2018 Twitter, Inc. Licensed under the Apache License, Version 2.0: https://www.apache.org/licenses/LICENSE-2.0 diff --git a/build.sbt b/build.sbt index b6bffe4179..4547ff8e21 100644 --- a/build.sbt +++ b/build.sbt @@ -446,7 +446,7 @@ lazy val injectThriftClient = (project in file("inject/inject-thrift-client")) .settings( name := "inject-thrift-client", moduleName := "inject-thrift-client", - ScoverageKeys.coverageExcludedPackages := ";.*\\.thriftscala.*;.*\\.thriftjava.*", + ScoverageKeys.coverageExcludedPackages := ";.*\\.thriftscala.*;.*\\.thriftjava.*;.*LatencyFilter.*", libraryDependencies ++= Seq( "com.twitter" %% "finagle-exp" % versions.twLibVersion, "com.twitter" %% "finagle-thrift" % versions.twLibVersion, diff --git a/doc/README.md b/doc/README.md index b01367e76d..edb6f7574f 100644 --- a/doc/README.md +++ b/doc/README.md @@ -17,9 +17,9 @@ $ ./pushsite.bash

-#### Copyright 2013-2017 Twitter, Inc. +#### Copyright 2013-2018 Twitter, Inc. Licensed under the Apache License, Version 2.0: http://www.apache.org/licenses/LICENSE-2.0 [twitter-server]: https://github.com/twitter/twitter-server -[finagle]: https://github.com/twitter/finagle \ No newline at end of file +[finagle]: https://github.com/twitter/finagle diff --git a/doc/src/sphinx/user-guide/http/clients.rst b/doc/src/sphinx/user-guide/http/clients.rst new file mode 100644 index 0000000000..22630daf05 --- /dev/null +++ b/doc/src/sphinx/user-guide/http/clients.rst @@ -0,0 +1,6 @@ +.. _http_clients: + +Communicate with an HTTP Service +================================ + +*COMING SOON* diff --git a/doc/src/sphinx/user-guide/index.rst b/doc/src/sphinx/user-guide/index.rst index 271bb9cc23..2104f737ca 100644 --- a/doc/src/sphinx/user-guide/index.rst +++ b/doc/src/sphinx/user-guide/index.rst @@ -66,6 +66,11 @@ Thrift Server - :doc:`thrift/exceptions` - :doc:`thrift/warmup` +Clients +------- + +- :doc:`thrift/clients` + Testing ------- @@ -114,6 +119,7 @@ Testing thrift/filters thrift/exceptions thrift/warmup + thrift/clients testing/index testing/embedded testing/feature_tests diff --git a/doc/src/sphinx/user-guide/thrift/clients.rst b/doc/src/sphinx/user-guide/thrift/clients.rst new file mode 100644 index 0000000000..e508f46aaa --- /dev/null +++ b/doc/src/sphinx/user-guide/thrift/clients.rst @@ -0,0 +1,270 @@ +.. _thrift_clients: + +Communicate with a Thrift Service +================================= + +Finatra provides support for integrating with `Scrooge `__-generated +`Finagle Thrift clients `__. + +As mentioned in the Scrooge `Finagle Integration `__ +documentation, users have three API choices for building an interface to a Finagle Thrift client — +``ServicePerEndpoint``, ``ReqRepServicePerEndpoint``, and ``MethodPerEndpoint``. + +The ``ServicePerEndpoint`` interface is a collection of Finagle |Services|_ where each Thrift method +is represented as a Finagle `Service[method.Args, method.SuccessType]`. By their nature of being +|Services|_, the methods are composable with Finagle's |Filters|_. + +The ``ReqRepServicePerEndpoint`` interface is also a collection of Finagle |Services|_, however +methods are |Services|_ from a |c.t.scrooge.Request|_ to a |c.t.scrooge.Response|_, e.g., +`Service[Request[method.Args], Response[method.SuccessType]]`. These envelope types allow for the +passing of header information between clients and servers when using the `com.twitter.finagle.ThriftMux` +protocol. + +Getting Started +--------------- + +To start, add a dependency on the Finatra `inject-thrift-client `__ library. + +E.g., with sbt: + +.. parsed-literal:: + + "com.twitter" %% "inject-thrift-client" % "\ |release|\ ", + +|ServicePerEndpointModule|_ +---------------------------------------------- + +.. note:: Currently the |c.t.inject.thrift.modules.ServicePerEndpointModule|_ only supports the +ThriftMux protocol. + +The |c.t.inject.thrift.modules.ServicePerEndpointModule|_ allows for configuration of a Finagle +Thrift client using the Finagle ThriftMux `MethodBuilder `__ +integration. + +Users have the option to configure a `service-per-endpoint`, i.e., ``ServicePerEndpoint`` or +``ReqRepServicePerEndpoint`` interface of a Thrift client and the |c.t.inject.thrift.modules.ServicePerEndpointModule|_ +provides bindings for both the chosen `service-per-endpoint` interface and the ``MethodPerEndpoint`` +interface. In this case, the ``MethodPerEndpoint`` interface functions as a thin wrapper the +configured `service-per-endpoint`. + +The choice to interact with Finagle |Services|_ when calling the configured Thrift client or to +use the Thrift method interface is up to you. You will have access to both in the object graph when +implementing a |c.t.inject.thrift.modules.ServicePerEndpointModule|_. + +To create a new client, first create a new `TwitterModule <../getting-started/modules.html>`_ which +extends |c.t.inject.thrift.modules.ServicePerEndpointModule|_: + +.. code:: scala + + import com.twitter.inject.thrift.modules.ServicePerEndpointModule + import com.twitter.myservice.thriftscala.MyService + + object MyServiceModule + extends ServicePerEndpointModule[MyService.ServicePerEndpoint, MyService.MethodPerEndpoint] { + + override val dest = "flag!myservice-thrift-service" + override val label = "myservice-thrift-client" + } + +The |ServicePerEndpointModule|_ implementation must be typed to a Scrooge-generated `service-per-endpoint` +and the ``MethodPerEndpoint``. Users can choose either the `MyService.ServicePerEndpoint` interface +(as in the above example) or the `MyService.ReqRepServicePerEndpoint` interface depending on their +requirements: + +.. code:: scala + + object MyServiceModule + extends ServicePerEndpointModule[MyService.ReqRepServicePerEndpoint, MyService.MethodPerEndpoint] { + ... + +At a minimum, to use the |c.t.inject.thrift.modules.ServicePerEndpointModule|_, a ThriftMux +`client label `__ and String +`dest `__ MUST also be specified. + +See `here `__ for more information on Finagle +clients. + +Configuration +~~~~~~~~~~~~~ + +The |ServicePerEndpointModule|_ intends to allow users to configure ThriftMux client +semantics and apply filters per-method via the |c.t.inject.thrift.ThriftMethodBuilder|_ which is a +thin wrapper over the Finagle ThriftMux `MethodBuilder `__. + +To configure per-method semantics, override and provide an implementation for the +`ServicePerEndpointModule#configureServicePerEndpointModule` method. E.g., + +.. code:: scala + + import com.twitter.inject.Injector + import com.twitter.inject.thrift.ThriftMethodBuilderFactory + import com.twitter.inject.thrift.modules.ServicePerEndpointModule + import com.twitter.myservice.thriftscala.MyService + + object MyServiceModule + extends ServicePerEndpointModule[MyService.ServicePerEndpoint, MyService.MethodPerEndpoint] { + + override val dest = "flag!myservice-thrift-service" + override val label = "myservice-thrift-client" + + override def configureServicePerEndpoint( + injector: Injector, + builder: ThriftMethodBuilderFactory[MyService.ServicePerEndpoint], + servicePerEndpoint: MyService.ServicePerEndpoint + ): MyService.ServicePerEndpoint = { + + servicePerEndpoint + .withFoo( + builder.method(MyService.Foo) + .withTimeoutPerRequest(???) + .withTotalTimeout(???) + .withRetryForClassifier(???) + .filtered(new MyFooMethodFilter) + .service) + .withBar( + builder.method(MyService.Bar) + .filtered(new MyTypeAgnosticFilter) + .withRetryForClassifier(???) + .service) + } + } + +In this example we are configuring the given `servicePerEndpoint` by re-implementing the `Foo` and +`Bar` functions using a "builder"-like API. Each `Scrooge `__-generated +client-side `ServicePerEndpoint` provides a `withXXXX` function over every defined Thrift method that +allows users to replace the current implementation of the method with a new implementation. The +replacement must still be a correctly-typed Finagle `Service`. + +In the above example we replace the methods with implementations built up from a combination of +`MethodBuilder `__ functionality and +arbitrary filters ending with a call to `ThriftMethodBuilder#service` which materializes the +resultant `Service`. + +Note that `TypeAgnostic `__ +Finagle |Filters|_ can also be applied "globally" across the all methods of a `ServicePerEndpoint` +interface by calling ``ServicePerEndpoint#filtered``. + +For example, to apply a set of `TypeAgnostic `__ +Finagle Filters to a `ServicePerEndpoint`: + +.. code:: scala + + import com.twitter.inject.Injector + import com.twitter.inject.thrift.ThriftMethodBuilderFactory + import com.twitter.inject.thrift.modules.ServicePerEndpointModule + import com.twitter.myservice.thriftscala.MyService + + object MyServiceModule + extends ServicePerEndpointModule[MyService.ServicePerEndpoint, MyService.MethodPerEndpoint] { + + override val dest = "flag!myservice-thrift-service" + override val label = "myservice-thrift-client" + + override def configureServicePerEndpoint( + injector: Injector, + builder: ThriftMethodBuilderFactory[MyService.ServicePerEndpoint], + servicePerEndpoint: MyService.ServicePerEndpoint + ): MyService.ServicePerEndpoint = { + + servicePerEndpoint + .filtered(???) + } + } + +This can be combined with the per-method configuration as well. + +.. admonition:: More Information on `Modules <../getting-started/modules.html>`__: + + Module `best practices <../getting-started/modules.html#best-practices>`__ + and `depending on other modules <../getting-started/modules.html#modules-depending-on-other-modules>`__. + +Bindings +~~~~~~~~ + +When included in a server's `module <../getting-started/modules.html>`__ list, an implementation +of the |ServicePerEndpointModule|_ will provide bindings to both +`MyService.ServicePerEndpoint `__ (or +`MyService.ReqRepServicePerEndpoint `__) **and** +`MyService.MethodPerEndpoint `__. + +For example, given the following |ServicePerEndpointModule|_ implementation: + +.. code:: scala + + import com.twitter.inject.thrift.modules.ServicePerEndpointModule + import com.twitter.myservice.thriftscala.MyService + + object MyServiceModule + extends ServicePerEndpointModule[MyService.ServicePerEndpoint, MyService.MethodPerEndpoint] { + + override val dest = "flag!myservice-thrift-service" + override val label = "myservice-thrift-client" + } + +This means that both `MyService.ServicePerEndpoint` and `MyService.MethodPerEndpoint` will be +injectable. Which to use is dependent on your use-case. + +.. code:: scala + + import com.twitter.finagle.http.Request + import com.twitter.finatra.http.Controller + import com.twitter.myservice.thriftscala.MyService + import javax.inject.{Inject, Singleton} + + @Singleton + class MyDataController @Inject()( + myService: MyService.MethodPerEndpoint + ) extends Controller { + get("/") { request: Request => + myService.foo(request.params("data")) + } + } + +More Information +---------------- + +For more information, see the `Finagle Integration `__ +section of the `Scrooge `__ documentation. + +More detailed examples are available in the integration tests: + +- |DoEverythingServicePerEndpointModuleFeatureTest|_ and +- |DoEverythingReqRepServicePerEndpointModuleFeatureTest|_ + +which test over two different implementations of a |ServicePerEndpointModule|_: + +- |GreeterServicePerEndpointModule|_ and +- |GreeterReqRepServicePerEndpointModule|_ respectively. + +.. |Services| replace:: `Services` +.. _Services: https://twitter.github.io/finagle/guide/ServicesAndFilters.html#services + +.. |Filters| replace:: `Filters` +.. _Filters: https://twitter.github.io/finagle/guide/ServicesAndFilters.html#filters + +.. |c.t.scrooge.Request| replace:: `c.t.scrooge.Request` +.. _c.t.scrooge.Request: https://github.com/twitter/scrooge/blob/develop/scrooge-core/src/main/scala/com/twitter/scrooge/Request.scala + +.. |c.t.scrooge.Response| replace:: `c.t.scrooge.Response` +.. _c.t.scrooge.Response: https://github.com/twitter/scrooge/blob/develop/scrooge-core/src/main/scala/com/twitter/scrooge/Response.scala + +.. |c.t.inject.thrift.ThriftMethodBuilder| replace:: ``c.t.inject.thrift.ThriftMethodBuilder`` +.. _c.t.inject.thrift.ThriftMethodBuilder: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/ThriftMethodBuilder.scala + +.. |c.t.inject.thrift.modules.ServicePerEndpointModule| replace:: `c.t.inject.thrift.modules.ServicePerEndpointModule` +.. _c.t.inject.thrift.modules.ServicePerEndpointModule: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ServicePerEndpointModule.scala + +.. |ServicePerEndpointModule| replace:: `ServicePerEndpointModule` +.. _ServicePerEndpointModule: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ServicePerEndpointModule.scala + +.. |DoEverythingServicePerEndpointModuleFeatureTest| replace:: `DoEverythingServicePerEndpointModuleFeatureTest` +.. _DoEverythingServicePerEndpointModuleFeatureTest: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingServicePerEndpointModuleFeatureTest.scala + +.. |GreeterServicePerEndpointModule| replace:: `GreeterServicePerEndpointModule` +.. _GreeterServicePerEndpointModule: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterServicePerEndpointModule.scala + +.. |DoEverythingReqRepServicePerEndpointModuleFeatureTest| replace:: `DoEverythingReqRepServicePerEndpointModuleFeatureTest` +.. _DoEverythingReqRepServicePerEndpointModuleFeatureTest: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingReqRepServicePerEndpointModuleFeatureTest.scala + +.. |GreeterReqRepServicePerEndpointModule| replace:: `GreeterReqRepServicePerEndpointModule` +.. _GreeterReqRepServicePerEndpointModule: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepServicePerEndpointModule.scala diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/ThriftMethodBuilder.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/ThriftMethodBuilder.scala new file mode 100644 index 0000000000..8ba376da02 --- /dev/null +++ b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/ThriftMethodBuilder.scala @@ -0,0 +1,253 @@ +package com.twitter.inject.thrift + +import com.twitter.finagle.service.ResponseClassifier +import com.twitter.finagle.thrift.service.{Filterable, ReqRepServicePerEndpointBuilder, ServicePerEndpointBuilder} +import com.twitter.finagle.thriftmux.MethodBuilder +import com.twitter.finagle.{Filter, Service} +import com.twitter.inject.Injector +import com.twitter.inject.thrift.internal.filters.ThriftClientExceptionFilter +import com.twitter.scrooge.ThriftMethod +import com.twitter.util.Duration +import com.twitter.util.tunable.Tunable + +/** + * A builder for configuring a Finagle [[Service]] representation of a ThriftMethod. See: + * [[https://twitter.github.io/scrooge/Finagle.html#id2 ServicePerEndpoint Client]] for more information. + * + * Filter ordering: + * {{{ + * Request Response + * | A + * | | + * V | + * +------------------------------------------------+ + * | ThriftMethodBuilder filters | + * +------------------------------------------------+ + * | MethodBuilder filters | + * +------------------------------------------------+ + * | Method Service | + * +------------------------------------------------+ + * }}} + * + */ +final class ThriftMethodBuilderFactory[ServicePerEndpoint <: Filterable[ServicePerEndpoint]]( + injector: Injector, + methodBuilder: MethodBuilder +)( + implicit builder: ServicePerEndpointBuilder[ServicePerEndpoint] +) { + + def method[Req, Rep](method: ThriftMethod): ThriftMethodBuilder[ServicePerEndpoint, Req, Rep] = + new ThriftMethodBuilder[ServicePerEndpoint, Req, Rep]( + injector, + methodBuilder, + method + ) +} + +/** + * Provides `ThriftMethod`-specific [[MethodBuilder]] functionality. + * @param mb the underlying [[MethodBuilder]] + * @param method the [[ThriftMethod]] over which configuration is applied. + * + * {{{ + * +-------------------------------------------------+ + * | ThriftMethodBuilder Exception filter | + * +-------------------------------------------------+ + * | ThriftMethodBuilder filters (in order added) | + * +-------------------------------------------------+ + * | MethodBuilder filters | + * +-------------------------------------------------+ + * }}} + */ +final class ThriftMethodBuilder[ServicePerEndpoint <: Filterable[ServicePerEndpoint], Req, Rep] private[thrift] ( + injector: Injector, + mb: MethodBuilder, + method: ThriftMethod +)( + implicit builder: ServicePerEndpointBuilder[ServicePerEndpoint] +) { + + // Mutable + private[this] var methodBuilder = mb + private[this] var filterChain: Filter[Req, Rep, Req, Rep] = Filter.identity + private[this] var exceptionFilterImpl: Filter[Req, Rep, Req, Rep] = + new ThriftClientExceptionFilter[Req, Rep](mb.label, method) + + /** + * @see [[com.twitter.finagle.thriftmux.MethodBuilder.withTimeoutTotal(howLong: Duration)]] + */ + def withTimeoutTotal(howLong: Duration): this.type = { + methodBuilder = methodBuilder.withTimeoutTotal(howLong) + this + } + + /** + * @see [[com.twitter.finagle.thriftmux.MethodBuilder.withTimeoutTotal(howLong: Tunable[Duration])]] + */ + def withTimeoutTotal(howLong: Tunable[Duration]): this.type = { + methodBuilder = methodBuilder.withTimeoutTotal(howLong) + this + } + + /** + * @see [[com.twitter.finagle.thriftmux.MethodBuilder.withTimeoutPerRequest(howLong: Duration)]] + */ + def withTimeoutPerRequest(howLong: Duration): this.type = { + methodBuilder = methodBuilder.withTimeoutPerRequest(howLong) + this + } + + /** + * @see [[com.twitter.finagle.thriftmux.MethodBuilder.withTimeoutPerRequest(howLong: Tunable[Duration])]] + */ + def withTimeoutPerRequest(howLong: Tunable[Duration]): this.type = { + methodBuilder = methodBuilder.withTimeoutPerRequest(howLong) + this + } + + /** + * @see [[com.twitter.finagle.thriftmux.MethodBuilder.withRetryForClassifier(classifier: ResponseClassifier)]] + */ + def withRetryForClassifier(classifier: ResponseClassifier): this.type = { + methodBuilder = methodBuilder.withRetryForClassifier(classifier) + this + } + + /** + * @see [[com.twitter.finagle.thriftmux.MethodBuilder.withRetryDisabled]] + */ + def withRetryDisabled: this.type = { + methodBuilder = methodBuilder.withRetryDisabled + this + } + + /** + * Install a [[com.twitter.finagle.Filter]]. This filter will be added to the end of the filter chain. That is, this + * filter will be invoked AFTER any other installed filter on a request [[Req]] and thus BEFORE any other installed + * filter on a response [[Rep]]. + * + * @param filter the [[com.twitter.finagle.Filter]] to install. + * @return [[ThriftMethodBuilder]] + */ + def filtered(filter: Filter[Req, Rep, Req, Rep]): this.type = { + filterChain = filterChain.andThen(filter) + this + } + + /** + * Install a [[com.twitter.finagle.Filter]]. This filter will be added to the end of the filter chain. That is, this + * filter will be invoked AFTER any other installed filter on a request [[Req]] and thus BEFORE any other installed + * filter on a response [[Rep]]. + * + * @tparam T [[Filter]] subtype of the filter to instantiate from the injector + * @return [[ThriftMethodBuilder]] + */ + def filtered[T <: Filter[Req, Rep, Req, Rep]: Manifest]: this.type = + filtered(injector.instance[T]) + + /** + * Install a [[com.twitter.finagle.Filter]] that is agnostic to the [[ThriftMethod]] Req/Rep types. This allows for + * use of more general filters that do not care about the [[ThriftMethod]] input and output types. + * + * @param filter the [[com.twitter.finagle.Filter.TypeAgnostic]] to install. + * @return [[ThriftMethodBuilder]] + */ + def withAgnosticFilter(filter: Filter.TypeAgnostic): this.type = { + filterChain = filterChain.andThen(filter.toFilter[Req, Rep]) + this + } + + /** + * Install a [[com.twitter.finagle.Filter.TypeAgnostic]] that is agnostic to the [[ThriftMethod]] Req/Rep types. This allows for + * use of more general filters that do not care about the [[ThriftMethod]] input and output types. + * + * @tparam T [[Filter.TypeAgnostic]] subtype of the filter to instantiate from the injector + * @return [[ThriftMethodBuilder]] + */ + def withAgnosticFilter[T <: Filter.TypeAgnostic: Manifest]: this.type = { + withAgnosticFilter(injector.instance[T]) + } + + /** + * Install a [[com.twitter.finagle.Filter]] specific to handling exceptions. This filter will be correctly positioned + * in the filter chain near the top of the stack. This filter is generally used to mutate or alter the final response + * [[Rep]] based on a returned exception. E.g., to translate a transport-level exception from Finagle to an + * application-level exception. + * + * @param filter the [[com.twitter.finagle.Filter]] to install. + * @return [[ThriftMethodBuilder]] + */ + def withExceptionFilter(filter: Filter[Req, Rep, Req, Rep]): this.type = { + exceptionFilterImpl = filter + this + } + + /** + * Install a [[com.twitter.finagle.Filter]] specific to handling exceptions. This filter will be correctly positioned + * in the filter chain near the top of the stack. This filter is generally used to mutate or alter the final response + * [[Rep]] based on a returned exception. E.g., to translate a transport-level exception from Finagle to an + * application-level exception. + * + * @tparam T the type of the filter to instantiate from the injector + * @return [[ThriftMethodBuilder]] + */ + def withExceptionFilter[T <: Filter[Req, Rep, Req, Rep]: Manifest]: this.type = { + withExceptionFilter(injector.instance[T]) + } + + /** + * Method-specific Filters are 'outside' of TypeAgnostic *and* the MethodBuilder filters + * + * The layer of indirection via the `AndThenService` is to allow for implementations that may wish + * to intercept the invocation of the service. + * + * @return a [[com.twitter.finagle.Service]] configured from the applied MethodBuilder configuration + * and filters. + */ + def service: Service[Req, Rep] = { + val methodServiceImpl: Service[Req, Rep] = findMethodService( + methodBuilder + .servicePerEndpoint[ServicePerEndpoint](method.name) + ) + + val service = builder match { + case _: ReqRepServicePerEndpointBuilder[ServicePerEndpoint] => + method + .toReqRepServicePerEndpointService( + methodServiceImpl.asInstanceOf[method.ReqRepFunctionType] + ) + .asInstanceOf[Service[Req, Rep]] + case _ => + method + .toServicePerEndpointService(methodServiceImpl.asInstanceOf[method.FunctionType]) + .asInstanceOf[Service[Req, Rep]] + } + toFilter.andThen(service) + } + + /* Private */ + + private[thrift] def toFilter: Filter[Req, Rep, Req, Rep] = { + exceptionFilterImpl + .andThen(filterChain) + } + + /* Find the given Service[T, U] method defined in the given implementation */ + private[this] def findMethodService[I <: Filterable[I]]( + implementation: I + ): Service[Req, Rep] = { + val methodOpt = implementation.getClass.getDeclaredMethods + .find(_.getName == method.name) + methodOpt match { + case Some(serviceMethod) => + serviceMethod + .invoke(implementation) + .asInstanceOf[Service[Req, Rep]] + case _ => + throw new IllegalArgumentException( + s"ThriftMethod: ${method.name} not found in implementation: ${implementation.getClass.getName}" + ) + } + } +} diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterBuilder.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterBuilder.scala index 3aee48faa0..92da0037d1 100644 --- a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterBuilder.scala +++ b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterBuilder.scala @@ -6,6 +6,7 @@ import com.twitter.inject.Injector import com.twitter.inject.thrift.AndThenService import com.twitter.scrooge.ThriftMethod +@deprecated("Use ServicePerEndpointModule and ThriftMethodBuilder", "2018-01-12") class ThriftClientFilterBuilder( timeoutMultiplier: Int, retryMultiplier: Int, diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterChain.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterChain.scala index d4f49b5c2e..2145f8d4e3 100644 --- a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterChain.scala +++ b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterChain.scala @@ -70,6 +70,7 @@ import org.joda.time.Duration * @see [[com.twitter.inject.thrift.filters.ThriftClientFilterBuilder]] * @see [[com.twitter.finagle.thrift.ThriftServiceIface]] */ +@deprecated("Use ServicePerEndpointModule and ThriftMethodBuilder", "2018-01-12") class ThriftClientFilterChain[Req <: ThriftStruct, Rep]( injector: Injector, statsReceiver: StatsReceiver, diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/internal/DefaultAndThenServiceImpl.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/internal/DefaultAndThenServiceImpl.scala index f1b3d6d2e3..171ee87c7a 100644 --- a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/internal/DefaultAndThenServiceImpl.scala +++ b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/internal/DefaultAndThenServiceImpl.scala @@ -5,7 +5,6 @@ import com.twitter.inject.thrift.AndThenService import com.twitter.scrooge.{ThriftMethod, ThriftStruct} private[thrift] class DefaultAndThenServiceImpl extends AndThenService { - def andThen[Req <: ThriftStruct, Rep]( method: ThriftMethod, filter: Filter[Req, Rep, Req, Rep], diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/internal/filters/ThriftClientExceptionFilter.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/internal/filters/ThriftClientExceptionFilter.scala index 0da96201be..c2d1940f4b 100644 --- a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/internal/filters/ThriftClientExceptionFilter.scala +++ b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/internal/filters/ThriftClientExceptionFilter.scala @@ -3,10 +3,10 @@ package com.twitter.inject.thrift.internal.filters import com.twitter.finagle.{Service, SimpleFilter} import com.twitter.inject.exceptions.PossiblyRetryable._ import com.twitter.inject.thrift.ThriftClientException -import com.twitter.scrooge.{ThriftMethod, ThriftStruct} +import com.twitter.scrooge.ThriftMethod import com.twitter.util._ -private[thrift] class ThriftClientExceptionFilter[Req <: ThriftStruct, Rep]( +private[thrift] final class ThriftClientExceptionFilter[Req, Rep]( clientLabel: String, method: ThriftMethod ) extends SimpleFilter[Req, Rep] { diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/FilteredThriftClientFlagsModule.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/FilteredThriftClientFlagsModule.scala index 61c099fa5b..3a4a5f2e4a 100644 --- a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/FilteredThriftClientFlagsModule.scala +++ b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/FilteredThriftClientFlagsModule.scala @@ -2,6 +2,7 @@ package com.twitter.inject.thrift.modules import com.twitter.inject.TwitterModule +@deprecated("No replacement. Timeouts can be manipulated with Tunables", "2018-01-08") object FilteredThriftClientFlagsModule extends TwitterModule { flag( "timeout.multiplier", diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/FilteredThriftClientModule.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/FilteredThriftClientModule.scala index 566888a7eb..5b9d26e6c6 100644 --- a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/FilteredThriftClientModule.scala +++ b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/FilteredThriftClientModule.scala @@ -63,6 +63,7 @@ object FilteredThriftClientModule { * @see [[https://finagle.github.io/blog/2015/09/10/services-per-endpoint-in-scrooge/ Services-per-endpoint in Scrooge]] * @see [[https://twitter.github.io/scrooge/Finagle.html#creating-a-client Finagle Clients]] */ +@deprecated("Use the com.twitter.inject.thrift.modules.ServicePerEndpointModule", "2018-01-08") abstract class FilteredThriftClientModule[ FutureIface <: ThriftService: ClassTag, ServiceIface <: ThriftServiceIface.Filterable[ServiceIface]: ClassTag diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ServicePerEndpointModule.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ServicePerEndpointModule.scala new file mode 100644 index 0000000000..cf600dfdbf --- /dev/null +++ b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ServicePerEndpointModule.scala @@ -0,0 +1,231 @@ +package com.twitter.inject.thrift.modules + +import com.google.inject.Provides +import com.twitter.finagle.service.Retries.Budget +import com.twitter.finagle.service.{ReqRep, ResponseClass, ResponseClassifier} +import com.twitter.finagle.stats.StatsReceiver +import com.twitter.finagle.thrift.ClientId +import com.twitter.finagle.thrift.service.{Filterable, MethodPerEndpointBuilder, ServicePerEndpointBuilder} +import com.twitter.finagle.{ThriftMux, thriftmux} +import com.twitter.inject.exceptions.PossiblyRetryable +import com.twitter.inject.thrift.ThriftMethodBuilderFactory +import com.twitter.inject.{Injector, TwitterModule} +import com.twitter.util.{Duration, Monitor, NullMonitor, Return, Throw} +import javax.inject.Singleton + +/** + * Provides bindings for a Scrooge-generated `ServicePerEndpoint` and `MethodPerEndpoint`. The + * `MethodPerEndpoint` is constructed via the [[MethodPerEndpointBuilder]] and is thus implemented + * as a thin wrapper over the `ServicePerEndpoint`. + * + * This [[TwitterModule]] allows users to configure a Scrooge-generated `ServicePerEndpoint` which + * can then be used directly or can be wrapped by a `MethodPerEndpoint`. + * + * Note when applying filters that filter order matters. The order in which filters are applied + * is the order in which requests will flow through to the service and the opposite of the order + * in which responses return. See the [[ThriftMethodBuilderFactory]] for more information. + * + * @tparam ServicePerEndpoint A Scrooge-generated ServicePerEndpoint + * @tparam MethodPerEndpoint A Scrooge-generated MethodPerEndpoint + */ +// TODO: remove deprecated ThriftClientModule and rename this to ThriftClientModule +abstract class ServicePerEndpointModule[ServicePerEndpoint <: Filterable[ServicePerEndpoint], MethodPerEndpoint]( + implicit servicePerEndpointBuilder: ServicePerEndpointBuilder[ServicePerEndpoint], + methodPerEndpointBuilder: MethodPerEndpointBuilder[ServicePerEndpoint, MethodPerEndpoint] +) extends TwitterModule { + + /** + * ThriftMux client label. + * @see [[https://twitter.github.io/finagle/guide/Clients.html#observability Clients Observability]] + */ + val label: String + + /** + * Destination of ThriftMux client. + * @see [[https://twitter.github.io/finagle/guide/Names.html Names and Naming in Finagle]] + */ + val dest: String + + /** + * Configures the session acquisition `timeout` of this client (default: unbounded). + * + * @return a [[Duration]] which represents the acquisition timeout + * @see [[com.twitter.finagle.param.ClientSessionParams.acquisitionTimeout]] + * @see [[https://twitter.github.io/finagle/guide/Clients.html#timeouts-expiration]] + */ + protected def sessionAcquisitionTimeout: Duration = Duration.Top + + /** + * Configures a "global" request `timeout` on the ThriftMux client (default: unbounded). + * Note there is the option to configure request timeouts per-method via configuring the + * [[com.twitter.inject.thrift.ThriftMethodBuilder]] for a given method. When applying + * configuration per-method it is recommended to configure timeouts per-method as well. + * + * However, this exists in the case you want *all* requests to *every* method to have + * the same total timeout. + * + * @return a [[Duration]] which represents the total request timeout + * @see [[com.twitter.finagle.param.CommonParams.withRequestTimeout]] + * @see [[https://twitter.github.io/finagle/guide/Clients.html#timeouts-expiration]] + * @see [[com.twitter.inject.thrift.ThriftMethodBuilder.withTimeoutPerRequest]] + * @see [[com.twitter.finagle.thriftmux.MethodBuilder.withTimeoutPerRequest]] + * @see [[com.twitter.inject.thrift.ThriftMethodBuilder.withTimeoutTotal]] + * @see [[com.twitter.finagle.thriftmux.MethodBuilder.withTimeoutTotal]] + */ + protected def requestTimeout: Duration = Duration.Top + + /** + * Default [[com.twitter.finagle.service.RetryBudget]]. It is highly recommended that budgets + * be shared between all filters that retry or re-queue requests to prevent retry storms. + * + * @return a default [[com.twitter.finagle.service.RetryBudget]] + * @see [[https://twitter.github.io/finagle/guide/Clients.html#retries]] + */ + protected def retryBudget: Budget = Budget.default + + /** + * Function to add a user-defined Monitor. A [[com.twitter.finagle.util.DefaultMonitor]] will be + * installed implicitly which handles all exceptions caught in the stack. Exceptions that are not + * handled by a user-defined monitor are propagated to the [[com.twitter.finagle.util.DefaultMonitor]]. + * + * NullMonitor has no influence on DefaultMonitor behavior here. + */ + protected def monitor: Monitor = NullMonitor + + @Provides + @Singleton + final def providesMethodPerEndpoint( + servicePerEndpoint: ServicePerEndpoint + ): MethodPerEndpoint = { + assert(thriftMuxClient != null, "Unexpected order of initialization.") + thriftMuxClient + .methodPerEndpoint[ServicePerEndpoint, MethodPerEndpoint](servicePerEndpoint) + } + + @Provides + @Singleton + final def providesServicePerEndpoint( + injector: Injector, + clientId: ClientId, + statsReceiver: StatsReceiver + ): ServicePerEndpoint = { + createThriftMuxClient(clientId, statsReceiver) + + val methodBuilder = + configureMethodBuilder( + thriftMuxClient.methodBuilder(dest)) + + configureServicePerEndpoint( + builder = new ThriftMethodBuilderFactory[ServicePerEndpoint]( + injector, + methodBuilder + ), + servicePerEndpoint = thriftMuxClient.servicePerEndpoint(dest, label) + ) + } + + /* Protected */ + + protected val PossiblyRetryableExceptions: ResponseClassifier = + ResponseClassifier.named("PossiblyRetryableExceptions") { + case ReqRep(_, Throw(t)) if PossiblyRetryable.possiblyRetryable(t) => + ResponseClass.RetryableFailure + case ReqRep(_, Throw(_)) => + ResponseClass.NonRetryableFailure + case ReqRep(_, Return(_)) => + ResponseClass.Success + } + + /** + * This method allows for extended configuration of the base MethodBuilder (e.g., the MethodBuilder + * used as a starting point for all method configurations) not exposed by this module or for + * overriding provided defaults, e.g., + * + * {{{ + * override def configureMethodBuilder(methodBuilder: thriftmux.MethodBuilder): thriftmux.MethodBuilder = { + * methodBuilder + * .withTimeoutTotal(5.seconds) + * } + * }}} + * + * Note: any configuration here will be applied to all methods unless explicitly overridden. However, + * also note that filters are cumulative. Thus filters added here will be present in any final configuration. + * + * @param methodBuilder the [[thriftmux.MethodBuilder]] to configure. + * @return a configured MethodBuilder which will be used as the starting point for any per-method + * configuration. + */ + protected def configureMethodBuilder( + methodBuilder: thriftmux.MethodBuilder + ): thriftmux.MethodBuilder = methodBuilder + + /** + * This method allows for further configuration of the ThriftMux client for parameters not exposed by + * this module or for overriding defaults provided herein, e.g., + * + * {{{ + * override def configureThriftMuxClient(client: ThriftMux.Client): ThriftMux.Client = { + * client + * .withProtocolFactory(myCustomProtocolFactory)) + * .withStatsReceiver(someOtherScopedStatsReceiver) + * .withMonitor(myAwesomeMonitor) + * .withTracer(notTheDefaultTracer) + * .withResponseClassifier(ThriftResponseClassifier.ThriftExceptionsAsFailures) + * } + *}}} + * + * @param client the [[com.twitter.finagle.ThriftMux.Client]] to configure. + * @return a configured ThriftMux.Client. + */ + protected def configureThriftMuxClient( + client: ThriftMux.Client + ): ThriftMux.Client = client + + /** + * Configure the ServicePerEndpoint. This is done by using the given [[ThriftMethodBuilderFactory]] + * to configure a [[com.twitter.inject.thrift.ThriftMethodBuilder]] for a given ThriftMethod. E.g., + * + * servicePerEndpoint + * .withFetchBlob( + * builder.method(FetchBlob) + * ... + * + * Subclasses of this module MAY provide an implementation of `configureServicePerEndpoint` which + * specifies configuration of a `ServicePerEndpoint` interface per-method of the interface. + * + * @param builder a [[ThriftMethodBuilderFactory]] for creating a [[com.twitter.inject.thrift.ThriftMethodBuilder]]. + * @param servicePerEndpoint the [[ServicePerEndpoint]] to configure. + * @return a per-method filtered [[ServicePerEndpoint]] + * @see [[com.twitter.inject.thrift.ThriftMethodBuilder]] + */ + protected def configureServicePerEndpoint( + builder: ThriftMethodBuilderFactory[ServicePerEndpoint], + servicePerEndpoint: ServicePerEndpoint + ): ServicePerEndpoint = servicePerEndpoint + + /* Private */ + + // We want each module to be able to configure a ThriftMux.client independently, + // however we do not want the client to be exposed in the object graph as including + // multiple modules in a server would attempt to bind the same type multiple times + // which would fail. Thus we use mutation to create and configure a ThriftMux.Client. + private[this] var thriftMuxClient: ThriftMux.Client = _ + private[this] def createThriftMuxClient( + clientId: ClientId, + statsReceiver: StatsReceiver + ): Unit = { + val clientStatsReceiver = statsReceiver.scope("clnt") + + thriftMuxClient = configureThriftMuxClient( + ThriftMux.client.withSession + .acquisitionTimeout(sessionAcquisitionTimeout) + .withRequestTimeout(requestTimeout) + .withStatsReceiver(clientStatsReceiver) + .withClientId(clientId) + .withMonitor(monitor) + .withLabel(label) + .withRetryBudget(retryBudget.retryBudget) + .withRetryBackoff(retryBudget.requeueBackoffs) + ) + } +} diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftClientModule.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftClientModule.scala index 374d490b04..9b9d0be32a 100644 --- a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftClientModule.scala +++ b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftClientModule.scala @@ -13,7 +13,7 @@ import com.twitter.util.Duration import javax.inject.Singleton import scala.reflect.ClassTag -@deprecated("Use the com.twitter.inject.thrift.modules.FilteredThriftClientModule", "2016-06-23") +@deprecated("Use the com.twitter.inject.thrift.modules.ServicePerEndpointModule", "2018-01-12") abstract class ThriftClientModule[T: ClassTag] extends TwitterModule with time.Implicits { /** diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingFilteredThriftClientModuleFeatureTest.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingFilteredThriftClientModuleFeatureTest.scala index 9fabacab62..6c9240cb8f 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingFilteredThriftClientModuleFeatureTest.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingFilteredThriftClientModuleFeatureTest.scala @@ -3,62 +3,70 @@ package com.twitter.inject.thrift import com.twitter.finagle.http.Status._ import com.twitter.finatra.http.{EmbeddedHttpServer, HttpTest} import com.twitter.finatra.thrift.EmbeddedThriftServer -import com.twitter.inject.Test -import com.twitter.inject.thrift.filtered_integration.http_server.GreeterHttpServer -import com.twitter.inject.thrift.filtered_integration.thrift_server.GreeterThriftServer +import com.twitter.inject.server.FeatureTest +import com.twitter.inject.thrift.integration.filtered.{GreeterFilteredThriftClientModule, GreeterHttpController, GreeterThriftService} +import com.twitter.inject.thrift.integration.{TestHttpServer, TestThriftServer} -class DoEverythingFilteredThriftClientModuleFeatureTest extends Test with HttpTest { +class DoEverythingFilteredThriftClientModuleFeatureTest + extends FeatureTest + with HttpTest { + override val printStats = false - val thriftServer = new EmbeddedThriftServer(twitterServer = new GreeterThriftServer) + private val thriftServer = new EmbeddedThriftServer( + twitterServer = new TestThriftServer(new GreeterThriftService) + ) - val httpServer = new EmbeddedHttpServer( - twitterServer = new GreeterHttpServer, + override val server = new EmbeddedHttpServer( + twitterServer = + new TestHttpServer[GreeterHttpController]( + "greeter-server", + GreeterFilteredThriftClientModule), args = Seq( "-thrift.clientId=greeter-http-service", - resolverMap("greeter-thrift-service" -> thriftServer.thriftHostAndPort) + resolverMap( + "greeter-thrift-service" -> thriftServer.thriftHostAndPort) ) ) override def afterAll() { - super.afterAll() - httpServer.close() thriftServer.close() + super.afterAll() } override protected def afterEach(): Unit = { - httpServer.clearStats() + server.clearStats() thriftServer.clearStats() super.afterEach() } - test("GreeterHttpServer#Say hi") { - httpServer.httpGet(path = "/hi?name=Bob", andExpect = Ok, withBody = "Hi Bob") + test("Say hi") { + server.httpGet(path = "/hi?name=Bob", andExpect = Ok, withBody = "Hi Bob") // per-method -- all the requests in this test were to the same method - httpServer.assertCounter("clnt/greeter-thrift-client/Greeter/hi/invocations", 1) + server.assertCounter("clnt/greeter-thrift-client/Greeter/hi/invocations", 1) /* assert counters added by ThriftServiceIface#statsFilter */ - httpServer.assertCounter("clnt/greeter-thrift-client/Greeter/hi/requests", 4) - httpServer.assertCounter("clnt/greeter-thrift-client/Greeter/hi/success", 2) - httpServer.assertCounter("clnt/greeter-thrift-client/Greeter/hi/failures", 2) + server.assertCounter("clnt/greeter-thrift-client/Greeter/hi/requests", 4) + server.assertCounter("clnt/greeter-thrift-client/Greeter/hi/success", 2) + server.assertCounter("clnt/greeter-thrift-client/Greeter/hi/failures", 2) /* assert latency stat exists */ - httpServer.getStat("clnt/greeter-thrift-client/Greeter/hi/latency_ms") should not be Seq() + server.getStat("clnt/greeter-thrift-client/Greeter/hi/latency_ms") should not be Seq() } - test("GreeterHttpServer#Say bye") { - httpServer.httpGet( + test("Say bye") { + server.httpGet( path = "/bye?name=Bob&age=18", andExpect = Ok, withBody = "Bye Bob of 18 years!" ) // per-method -- all the requests in this test were to the same method - httpServer.assertCounter("clnt/greeter-thrift-client/Greeter/bye/invocations", 1) + server.assertCounter("clnt/greeter-thrift-client/Greeter/bye/invocations", 1) /* assert counters added by StatsFilter */ - httpServer.assertCounter("clnt/greeter-thrift-client/Greeter/bye/requests", 3) - httpServer.assertCounter("clnt/greeter-thrift-client/Greeter/bye/success", 1) - httpServer.assertCounter("clnt/greeter-thrift-client/Greeter/bye/failures", 2) + server.assertCounter("clnt/greeter-thrift-client/Greeter/bye/requests", 3) + server.assertCounter("clnt/greeter-thrift-client/Greeter/bye/success", 1) + server.assertCounter("clnt/greeter-thrift-client/Greeter/bye/failures", 2) /* assert latency stat exists */ - httpServer.getStat("clnt/greeter-thrift-client/Greeter/bye/latency_ms") should not be Seq() - httpServer.getStat("clnt/greeter-thrift-client/Greeter/bye/request_latency_ms") should not be Seq() + server.getStat("clnt/greeter-thrift-client/Greeter/bye/latency_ms") should not be Seq() + server.getStat("clnt/greeter-thrift-client/Greeter/bye/request_latency_ms") should not be Seq() } } diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingReqRepServicePerEndpointModuleFeatureTest.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingReqRepServicePerEndpointModuleFeatureTest.scala new file mode 100644 index 0000000000..e2af474699 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingReqRepServicePerEndpointModuleFeatureTest.scala @@ -0,0 +1,117 @@ +package com.twitter.inject.thrift + +import com.twitter.conversions.time._ +import com.twitter.finagle.http.Status.Ok +import com.twitter.finatra.http.{EmbeddedHttpServer, HttpTest} +import com.twitter.finatra.thrift.EmbeddedThriftServer +import com.twitter.greeter.thriftscala.Greeter +import com.twitter.inject.server.FeatureTest +import com.twitter.inject.thrift.DoEverythingReqRepServicePerEndpointModuleFeatureTest._ +import com.twitter.inject.thrift.integration.reqrepserviceperendpoint.{ReqRepServicePerEndpointHttpController, GreeterReqRepServicePerEndpointModule, GreeterThriftService} +import com.twitter.inject.thrift.integration.{TestHttpServer, TestThriftServer} +import com.twitter.util.Duration +import com.twitter.util.tunable.Tunable + +object DoEverythingReqRepServicePerEndpointModuleFeatureTest { + case class HelloHeaders(empty: Boolean) + case class HelloResponse(value: String, headers: HelloHeaders) +} + +class DoEverythingReqRepServicePerEndpointModuleFeatureTest + extends FeatureTest + with HttpTest { + override val printStats = false + + private val requestHeaderKey = "com.twitter.greeter.test.header" + private val httpServiceClientId = "http-service" + private val perRequestTimeoutTunable: Tunable[Duration] = Tunable.mutable("per-request", 50.millis) + + private val greeterThriftServer = new EmbeddedThriftServer( + twitterServer = new TestThriftServer( + new GreeterThriftService( + httpServiceClientId, + requestHeaderKey).toThriftService) + ) + + override val server = new EmbeddedHttpServer( + twitterServer = new TestHttpServer[ReqRepServicePerEndpointHttpController]( + "rrspe-server", + new GreeterReqRepServicePerEndpointModule(requestHeaderKey, perRequestTimeoutTunable)), + args = Seq( + s"-thrift.clientId=$httpServiceClientId", + resolverMap( + "greeter-thrift-service" -> greeterThriftServer.thriftHostAndPort) + ) + ) + + override def afterAll() { + greeterThriftServer.close() + super.afterAll() + } + + test("Greeter.ReqRepServicePerEndpoint is available from the injector") { + server.injector.instance[Greeter.ReqRepServicePerEndpoint] should not be null + } + + test("Greeter.MethodPerEndpoint is available from the injector") { + server.injector.instance[Greeter.MethodPerEndpoint] should not be null + } + + test("Say hi") { + // fails more times (3) than the MethodBuilder number of retries (2). + intercept[Exception] { + server.httpGet(path = "/hi?name=Bob", andExpect = Ok, withBody = "Hi Bob") + } + + // per-method -- all the requests in this test were to the same method + /* assert counters added by ThriftServiceIface#statsFilter */ + server.assertCounter("clnt/greeter-thrift-client/Greeter/hi/requests", 3) + server.assertCounter("clnt/greeter-thrift-client/Greeter/hi/success", 1) + server.assertCounter("clnt/greeter-thrift-client/Greeter/hi/failures", 2) + /* assert MethodBuilder stats exist */ + server.getStat("clnt/greeter-thrift-client/hi/logical/request_latency_ms") should not be Seq() + server.getStat("clnt/greeter-thrift-client/hi/retries") should be(Seq(2.0)) + /* assert MethodBuilder counters */ + server.assertCounter("clnt/greeter-thrift-client/hi/logical/requests", 1) + server.assertCounter("clnt/greeter-thrift-client/hi/logical/success", 1) + } + + test("Say hello") { + val response = + server.httpGetJson[HelloResponse](path = "/hello?name=Bob", andExpect = Ok) + assert(response.value == "Hello Bob") + assert(!response.headers.empty) + + // per-method -- all the requests in this test were to the same method + /* assert counters added by ThriftServiceIface#statsFilter */ + server.assertCounter("clnt/greeter-thrift-client/Greeter/hello/requests", 3) + server.assertCounter("clnt/greeter-thrift-client/Greeter/hello/success", 1) + server.assertCounter("clnt/greeter-thrift-client/Greeter/hello/failures", 2) + /* assert MethodBuilder stats exist */ + server.getStat("clnt/greeter-thrift-client/hello/logical/request_latency_ms") should not be Seq() + server.getStat("clnt/greeter-thrift-client/hello/retries") should be(Seq(2.0)) + /* assert MethodBuilder counters */ + server.assertCounter("clnt/greeter-thrift-client/hello/logical/requests", 1) + server.assertCounter("clnt/greeter-thrift-client/hello/logical/success", 1) + } + + test("Say bye") { + server.httpGet( + path = "/bye?name=Bob&age=18", + andExpect = Ok, + withBody = "Bye Bob of 18 years!" + ) + + // per-method -- all the requests in this test were to the same method + /* assert counters added by ThriftServiceIface#statsFilter */ + server.assertCounter("clnt/greeter-thrift-client/Greeter/bye/requests", 3) + server.assertCounter("clnt/greeter-thrift-client/Greeter/bye/success", 1) + server.assertCounter("clnt/greeter-thrift-client/Greeter/bye/failures", 2) + /* assert MethodBuilder stats exist */ + server.getStat("clnt/greeter-thrift-client/bye/logical/request_latency_ms") should not be Seq() + server.getStat("clnt/greeter-thrift-client/bye/retries") should be(Seq(2.0)) + /* assert MethodBuilder counters */ + server.assertCounter("clnt/greeter-thrift-client/bye/logical/requests", 1) + server.assertCounter("clnt/greeter-thrift-client/bye/logical/success", 1) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingServicePerEndpointModuleFeatureTest.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingServicePerEndpointModuleFeatureTest.scala new file mode 100644 index 0000000000..742ce6dcc6 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingServicePerEndpointModuleFeatureTest.scala @@ -0,0 +1,163 @@ +package com.twitter.inject.thrift + +import com.twitter.finagle.http.Status.Ok +import com.twitter.finatra.http.{EmbeddedHttpServer, HttpTest} +import com.twitter.finatra.thrift.EmbeddedThriftServer +import com.twitter.greeter.thriftscala.Greeter +import com.twitter.inject.server.FeatureTest +import com.twitter.inject.thrift.integration.serviceperendpoint.{ + EchoServicePerEndpointModule, + EchoThriftService, + GreeterServicePerEndpointModule, + GreeterThriftService, + ServicePerEndpointHttpController +} +import com.twitter.inject.thrift.integration.{TestHttpServer, TestThriftServer} +import com.twitter.test.thriftscala.EchoService + +class DoEverythingServicePerEndpointModuleFeatureTest extends FeatureTest with HttpTest { + override val printStats = false + + private val httpServiceClientId = "http-service" + + private val greeterThriftServer = new EmbeddedThriftServer( + twitterServer = + new TestThriftServer(new GreeterThriftService(httpServiceClientId).toThriftService) + ) + + private val echoThriftServer = new EmbeddedThriftServer( + twitterServer = new TestThriftServer(new EchoThriftService(httpServiceClientId).toThriftService) + ) + + // Test an HttpServer with multiple clients + val server = new EmbeddedHttpServer( + twitterServer = new TestHttpServer[ServicePerEndpointHttpController]( + "spe-server", + GreeterServicePerEndpointModule, + EchoServicePerEndpointModule + ), + args = Seq( + s"-thrift.clientId=$httpServiceClientId", + resolverMap( + "greeter-thrift-service" -> greeterThriftServer.thriftHostAndPort, + "echo-thrift-service" -> echoThriftServer.thriftHostAndPort + ) + ) + ) + + override def afterAll() { + greeterThriftServer.close() + echoThriftServer.close() + super.afterAll() + } + + test("Greeter.ServicePerEndpoint is available from the injector") { + server.injector.instance[Greeter.ServicePerEndpoint] should not be null + } + + test("Greeter.MethodPerEndpoint is available from the injector") { + server.injector.instance[Greeter.MethodPerEndpoint] should not be null + } + + test("EchoService.ServicePerEndpoint is available from the injector") { + server.injector.instance[EchoService.ServicePerEndpoint] should not be null + } + + test("EchoService.MethodPerEndpoint is available from the injector") { + server.injector.instance[EchoService.MethodPerEndpoint] should not be null + } + + test("Say hi") { + // fails more times (3) than the MethodBuilder number of retries (2). + intercept[Exception] { + server.httpGet(path = "/hi?name=Bob", andExpect = Ok, withBody = "Hi Bob") + } + // http server route stat + server.assertStat("route/hi/GET/response_size", Seq(5)) + + // per-method -- all the requests in this test were to the same method + /* assert counters added by ThriftServiceIface#statsFilter */ + server.assertCounter("clnt/greeter-thrift-client/Greeter/hi/requests", 3) + server.assertCounter("clnt/greeter-thrift-client/Greeter/hi/success", 1) + server.assertCounter("clnt/greeter-thrift-client/Greeter/hi/failures", 2) + /* assert MethodBuilder stats exist */ + server.getStat("clnt/greeter-thrift-client/hi/logical/request_latency_ms") should not be Seq() + server.getStat("clnt/greeter-thrift-client/hi/retries") should be(Seq(2.0)) + /* assert MethodBuilder counters */ + server.assertCounter("clnt/greeter-thrift-client/hi/logical/requests", 1) + server.assertCounter("clnt/greeter-thrift-client/hi/logical/success", 1) + } + + test("Say hello") { + server.httpGet(path = "/hello?name=Bob", andExpect = Ok, withBody = "Hello Bob") + // http server route stat + server.assertStat("route/hello/GET/response_size", Seq(9)) + + // per-method -- all the requests in this test were to the same method + /* assert counters added by ThriftServiceIface#statsFilter */ + server.assertCounter("clnt/greeter-thrift-client/Greeter/hello/requests", 3) + server.assertCounter("clnt/greeter-thrift-client/Greeter/hello/success", 1) + server.assertCounter("clnt/greeter-thrift-client/Greeter/hello/failures", 2) + /* assert MethodBuilder stats exist */ + server.getStat("clnt/greeter-thrift-client/hello/logical/request_latency_ms") should not be Seq() + server.getStat("clnt/greeter-thrift-client/hello/retries") should be(Seq(2.0)) + /* assert MethodBuilder counters */ + server.assertCounter("clnt/greeter-thrift-client/hello/logical/requests", 1) + server.assertCounter("clnt/greeter-thrift-client/hello/logical/success", 1) + } + + test("Say bye") { + server.httpGet( + path = "/bye?name=Bob&age=18", + andExpect = Ok, + withBody = "Bye Bob of 18 years!" + ) + // http server route stat + server.assertStat("route/bye/GET/response_size", Seq(20)) + + // per-method -- all the requests in this test were to the same method + /* assert counters added by ThriftServiceIface#statsFilter */ + server.assertCounter("clnt/greeter-thrift-client/Greeter/bye/requests", 3) + server.assertCounter("clnt/greeter-thrift-client/Greeter/bye/success", 1) + server.assertCounter("clnt/greeter-thrift-client/Greeter/bye/failures", 2) + /* assert MethodBuilder stats exist */ + server.getStat("clnt/greeter-thrift-client/bye/logical/request_latency_ms") should not be Seq() + server.getStat("clnt/greeter-thrift-client/bye/retries") should be(Seq(2.0)) + /* assert MethodBuilder counters */ + server.assertCounter("clnt/greeter-thrift-client/bye/logical/requests", 1) + server.assertCounter("clnt/greeter-thrift-client/bye/logical/success", 1) + } + + test("echo 3 times") { + server.httpPost( + path = "/config?timesToEcho=2", + postBody = "", + andExpect = Ok, + withBody = "2" + ) + + server.httpPost( + path = "/config?timesToEcho=3", + postBody = "", + andExpect = Ok, + withBody = "3" + ) + + server.httpGet(path = "/echo?msg=Bob", andExpect = Ok, withBody = "BobBobBob") + // http server route stats + server.assertStat("route/config/POST/response_size", Seq(1, 1)) + server.assertStat("route/echo/GET/response_size", Seq(9)) + + // per-method -- all the requests in this test were to the same method + /* assert counters added by ThriftServiceIface#statsFilter */ + server.assertCounter("clnt/echo-thrift-client/EchoService/echo/requests", 1) + server.assertCounter("clnt/echo-thrift-client/EchoService/echo/success", 1) + server.assertCounter("clnt/echo-thrift-client/EchoService/echo/failures", 0) + /* assert MethodBuilder stats exist */ + server.getStat("clnt/echo-thrift-client/echo/logical/request_latency_ms") should not be Seq() + server.getStat("clnt/echo-thrift-client/echo/retries") should be(Seq(0.0)) + /* assert MethodBuilder counters */ + server.assertCounter("clnt/echo-thrift-client/echo/logical/requests", 1) + server.assertCounter("clnt/echo-thrift-client/echo/logical/success", 1) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingThriftClientModuleFeatureTest.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingThriftClientModuleFeatureTest.scala new file mode 100644 index 0000000000..f6d75cecff --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingThriftClientModuleFeatureTest.scala @@ -0,0 +1,186 @@ +package com.twitter.inject.thrift + +import com.twitter.conversions.time._ +import com.twitter.finagle.http.Request +import com.twitter.finagle.http.Status._ +import com.twitter.finagle.thrift.ClientId +import com.twitter.finagle.{ListeningServer, ThriftMux} +import com.twitter.finatra.http.filters.CommonFilters +import com.twitter.finatra.http.routing.HttpRouter +import com.twitter.finatra.http.{Controller, EmbeddedHttpServer, HttpServer, HttpTest} +import com.twitter.finatra.thrift.EmbeddedThriftServer +import com.twitter.inject.server.{PortUtils, TwitterServer} +import com.twitter.inject.thrift.DoEverythingThriftClientModuleFeatureTest._ +import com.twitter.inject.thrift.modules.{ThriftClientIdModule, ThriftClientModule} +import com.twitter.inject.{Logging, Test} +import com.twitter.test.thriftscala.EchoService +import com.twitter.util.{Await, Future} +import java.util.concurrent.atomic.AtomicInteger +import javax.inject.{Inject, Singleton} + +object DoEverythingThriftClientModuleFeatureTest { + class EchoHttpController1 @Inject()(echoThriftService: EchoService[Future]) extends Controller { + get("/echo") { request: Request => + val msg = request.params("msg") + echoThriftService.echo(msg) + } + post("/config") { request: Request => + val timesToEcho = request.params("timesToEcho").toInt + echoThriftService.setTimesToEcho(timesToEcho) + } + } + + class EchoHttpController2 @Inject()(echoThriftService: EchoService.FutureIface) extends Controller { + get("/echo") { request: Request => + val msg = request.params("msg") + echoThriftService.echo(msg) + } + post("/config") { request: Request => + val timesToEcho = request.params("timesToEcho").toInt + echoThriftService.setTimesToEcho(timesToEcho) + } + } + + object EchoThriftClientModule1 + extends ThriftClientModule[EchoService[Future]] { + override val label = "echo-service" + override val dest = "flag!thrift-echo-service" + } + + object EchoThriftClientModule2 + extends ThriftClientModule[EchoService.FutureIface] { + override val label = "echo-service" + override val dest = "flag!thrift-echo-service" + } + + @Singleton + class MyEchoService extends EchoService[Future] with Logging { + private val timesToEcho = new AtomicInteger(1) + + /* Public */ + override def echo(msg: String): Future[String] = { + info("echo " + msg) + assertClientId("echo-http-service") + Future.value(msg * timesToEcho.get) + } + override def setTimesToEcho(times: Int): Future[Int] = { + info("setTimesToEcho " + times) + assertClientId("echo-http-service") + timesToEcho.set(times) //mutation + Future(times) + } + + /* Private */ + private def assertClientId(name: String): Unit = { + assert(ClientId.current.contains(ClientId(name)), "Invalid Client ID: " + ClientId.current) + } + } + + class EchoThriftServer extends TwitterServer { + private val thriftPortFlag = flag("thrift.port", ":0", "External Thrift server port") + private val thriftShutdownTimeout = flag( + "thrift.shutdown.time", + 1.minute, + "Maximum amount of time to wait for pending requests to complete on shutdown" + ) + /* Private Mutable State */ + private var thriftServer: ListeningServer = _ + + /* Lifecycle */ + override def postWarmup() { + super.postWarmup() + thriftServer = ThriftMux.server.serveIface(thriftPortFlag(), injector.instance[MyEchoService]) + info("Thrift server started on port: " + thriftPort.get) + } + + onExit { + Await.result(thriftServer.close(thriftShutdownTimeout().fromNow)) + } + + /* Overrides */ + override def thriftPort = Option(thriftServer) map PortUtils.getPort + } +} + +class DoEverythingThriftClientModuleFeatureTest extends Test with HttpTest { + val thriftServer = + new EmbeddedThriftServer( + twitterServer = new EchoThriftServer) + + val httpServer1 = new EmbeddedHttpServer( + twitterServer = new HttpServer { + override val name = "echo-http-server1" + override val modules = Seq(ThriftClientIdModule, EchoThriftClientModule1) + override def configureHttp(router: HttpRouter) { + router.filter[CommonFilters].add[EchoHttpController1] + } + }, + args = Seq( + "-thrift.clientId=echo-http-service", + resolverMap("thrift-echo-service" -> thriftServer.thriftHostAndPort) + ) + ) + + val httpServer2 = new EmbeddedHttpServer( + twitterServer = new HttpServer { + override val name = "echo-http-server2" + override val modules = Seq(ThriftClientIdModule, EchoThriftClientModule2) + override def configureHttp(router: HttpRouter) { + router.filter[CommonFilters].add[EchoHttpController2] + } + }, + args = Seq( + "-thrift.clientId=echo-http-service", + resolverMap("thrift-echo-service" -> thriftServer.thriftHostAndPort) + ) + ) + + override def afterAll(): Unit = { + thriftServer.close() + httpServer1.close() + httpServer2.close() + super.afterAll() + } + + // test EchoService[Future] + test("EchoHttpServer1#echo 3 times") { + httpServer1.httpPost( + path = "/config?timesToEcho=2", + postBody = "", + andExpect = Ok, + withBody = "2" + ) + + httpServer1.httpPost( + path = "/config?timesToEcho=3", + postBody = "", + andExpect = Ok, + withBody = "3" + ) + + httpServer1.httpGet(path = "/echo?msg=Bob", andExpect = Ok, withBody = "BobBobBob") + httpServer1.assertStat("route/config/POST/response_size", Seq(1, 1)) + httpServer1.assertStat("route/echo/GET/response_size", Seq(9)) + } + + // test EchoService.FutureIface + test("EchoHttpServer2#echo 3 times") { + httpServer2.httpPost( + path = "/config?timesToEcho=2", + postBody = "", + andExpect = Ok, + withBody = "2" + ) + + httpServer2.httpPost( + path = "/config?timesToEcho=3", + postBody = "", + andExpect = Ok, + withBody = "3" + ) + + httpServer2.httpGet(path = "/echo?msg=Bob", andExpect = Ok, withBody = "BobBobBob") + httpServer2.assertStat("route/config/POST/response_size", Seq(1, 1)) + httpServer2.assertStat("route/echo/GET/response_size", Seq(9)) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/EchoHttpServerFeatureTest.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/EchoHttpServerFeatureTest.scala deleted file mode 100644 index 760044d831..0000000000 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/EchoHttpServerFeatureTest.scala +++ /dev/null @@ -1,45 +0,0 @@ -package com.twitter.inject.thrift - -import com.twitter.finagle.http.Status._ -import com.twitter.finatra.http.{EmbeddedHttpServer, HttpTest} -import com.twitter.finatra.thrift.EmbeddedThriftServer -import com.twitter.inject.Test -import com.twitter.inject.thrift.integration.http_server.EchoHttpServer -import com.twitter.inject.thrift.integration.thrift_server.EchoThriftServer - -class EchoHttpServerFeatureTest extends Test with HttpTest { - - val thriftServer = new EmbeddedThriftServer(twitterServer = new EchoThriftServer) - - val httpServer = new EmbeddedHttpServer( - twitterServer = new EchoHttpServer, - args = Seq( - "-thrift.clientId=echo-http-service", - resolverMap("thrift-echo-service" -> thriftServer.thriftHostAndPort) - ) - ) - - test("EchoHttpServer#echo 3 times") { - httpServer.httpPost( - path = "/config?timesToEcho=2", - postBody = "", - andExpect = Ok, - withBody = "2" - ) - - httpServer.httpPost( - path = "/config?timesToEcho=3", - postBody = "", - andExpect = Ok, - withBody = "3" - ) - - httpServer.httpGet(path = "/echo?msg=Bob", andExpect = Ok, withBody = "BobBobBob") - - httpServer.assertStat("route/config/POST/response_size", Seq(1, 1)) - httpServer.assertStat("route/echo/GET/response_size", Seq(9)) - - httpServer.close() - thriftServer.close() - } -} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/GreeterHttpServerFeatureTest.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/GreeterHttpServerFeatureTest.scala deleted file mode 100644 index 975a95d82d..0000000000 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/GreeterHttpServerFeatureTest.scala +++ /dev/null @@ -1,39 +0,0 @@ -package com.twitter.inject.thrift - -import com.twitter.finagle.http.Status._ -import com.twitter.finatra.http.{EmbeddedHttpServer, HttpTest} -import com.twitter.finatra.thrift.EmbeddedThriftServer -import com.twitter.inject.Test -import com.twitter.inject.thrift.filtered_integration.http_server.GreeterHttpServer -import com.twitter.inject.thrift.filtered_integration.thrift_server.GreeterThriftServer - -class GreeterHttpServerFeatureTest extends Test with HttpTest { - - val thriftServer = new EmbeddedThriftServer(twitterServer = new GreeterThriftServer) - - val httpServer = new EmbeddedHttpServer( - twitterServer = new GreeterHttpServer, - args = Seq( - "-thrift.clientId=greeter-http-service", - resolverMap("greeter-thrift-service" -> thriftServer.thriftHostAndPort) - ) - ) - - override def afterAll() { - super.afterAll() - httpServer.close() - thriftServer.close() - } - - test("GreeterHttpServer#Say hi") { - httpServer.httpGet(path = "/hi?name=Bob", andExpect = Ok, withBody = "Hi Bob") - } - - test("GreeterHttpServer#Say bye") { - httpServer.httpGet( - path = "/bye?name=Bob&age=18", - andExpect = Ok, - withBody = "Bye Bob of 18 years!" - ) - } -} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/InheritanceServicePerEndpointModuleFeatureTest.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/InheritanceServicePerEndpointModuleFeatureTest.scala new file mode 100644 index 0000000000..ae11f54c66 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/InheritanceServicePerEndpointModuleFeatureTest.scala @@ -0,0 +1,78 @@ +package com.twitter.inject.thrift + +import com.twitter.finagle.http.Status.Ok +import com.twitter.finatra.http.{EmbeddedHttpServer, HttpTest} +import com.twitter.finatra.thrift.EmbeddedThriftServer +import com.twitter.inject.server.FeatureTest +import com.twitter.inject.thrift.integration.TestThriftServer +import com.twitter.inject.thrift.integration.inheritance.{ServiceBHttpServer, ServiceBThriftService} +import com.twitter.serviceB.thriftscala.ServiceB + +class InheritanceServicePerEndpointModuleFeatureTest + extends FeatureTest + with HttpTest { + override val printStats = false + + private val httpClientId: String = "http-server" + + private val thriftServer = new EmbeddedThriftServer( + twitterServer = new TestThriftServer( + new ServiceBThriftService(httpClientId).toThriftService) + ) + + val server = new EmbeddedHttpServer( + twitterServer = + new ServiceBHttpServer, + args = Seq( + s"-thrift.clientId=$httpClientId", + resolverMap( + "serviceB-thrift-service" -> thriftServer.thriftHostAndPort) + ) + ) + + override def afterAll() { + thriftServer.close() + super.afterAll() + } + + test("ServiceB.ServicePerEndpoint is available from the injector") { + server.injector.instance[ServiceB.ServicePerEndpoint] should not be null + } + + test("ServiceB.MethodPerEndpoint is available from the injector") { + server.injector.instance[ServiceB.MethodPerEndpoint] should not be null + } + + test("echo") { + server.httpGet(path = "/echo?msg=Hello!", andExpect = Ok, withBody = "Hello!") + + // per-method -- all the requests in this test were to the same method + /* assert counters added by ThriftServiceIface#statsFilter */ + server.assertCounter("clnt/serviceB-thrift-client/ServiceA/echo/requests", 1) + server.assertCounter("clnt/serviceB-thrift-client/ServiceA/echo/success", 1) + server.assertCounter("clnt/serviceB-thrift-client/ServiceA/echo/failures", 0) + /* assert MethodBuilder stats exist */ + server.getStat("clnt/serviceB-thrift-client/echo/logical/request_latency_ms") should not be Seq() + server.getStat("clnt/serviceB-thrift-client/echo/retries") should be(Seq(0.0)) + /* assert MethodBuilder counters */ + server.assertCounter("clnt/serviceB-thrift-client/echo/logical/requests", 1) + server.assertCounter("clnt/serviceB-thrift-client/echo/logical/success", 1) + } + + test("ping") { + server.httpGet(path = "/ping", andExpect = Ok, withBody = "pong") + + // per-method -- all the requests in this test were to the same method + /* assert counters added by ThriftServiceIface#statsFilter */ + server.assertCounter("clnt/serviceB-thrift-client/ServiceB/ping/requests", 1) + server.assertCounter("clnt/serviceB-thrift-client/ServiceB/ping/success", 1) + server.assertCounter("clnt/serviceB-thrift-client/ServiceB/ping/failures", 0) + /* assert MethodBuilder stats exist */ + server.getStat("clnt/serviceB-thrift-client/ping/logical/request_latency_ms") should not be Seq() + // TODO: CSL-5842 + // server.getStat("clnt/serviceB-thrift-client/ping/retries") should be(Seq(0.0)) + /* assert MethodBuilder counters */ + server.assertCounter("clnt/serviceB-thrift-client/ping/logical/requests", 1) + server.assertCounter("clnt/serviceB-thrift-client/ping/logical/success", 1) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/GreeterHttpServer.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/GreeterHttpServer.scala deleted file mode 100644 index 59773f8066..0000000000 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/GreeterHttpServer.scala +++ /dev/null @@ -1,16 +0,0 @@ -package com.twitter.inject.thrift.filtered_integration.http_server - -import com.twitter.finatra.http.HttpServer -import com.twitter.finatra.http.filters.CommonFilters -import com.twitter.finatra.http.routing.HttpRouter -import com.twitter.inject.thrift.modules.ThriftClientIdModule - -class GreeterHttpServer extends HttpServer { - override val name = "greeter-server" - - override val modules = Seq(ThriftClientIdModule, GreeterFilteredThriftClientModule) - - override def configureHttp(router: HttpRouter) { - router.filter[CommonFilters].add[GreeterHttpController] - } -} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/AbstractThriftService.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/AbstractThriftService.scala new file mode 100644 index 0000000000..5cfc51524c --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/AbstractThriftService.scala @@ -0,0 +1,10 @@ +package com.twitter.inject.thrift.integration + +import com.twitter.finagle.thrift.ClientId + +abstract class AbstractThriftService { + + protected def assertClientId(name: String): Unit = { + assert(ClientId.current.contains(ClientId(name)), "Invalid Client ID: " + ClientId.current) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/TestHttpServer.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/TestHttpServer.scala new file mode 100644 index 0000000000..69296f04e8 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/TestHttpServer.scala @@ -0,0 +1,24 @@ +package com.twitter.inject.thrift.integration + +import com.google.inject.Module +import com.twitter.finatra.http.{Controller, HttpServer} +import com.twitter.finatra.http.filters.CommonFilters +import com.twitter.finatra.http.routing.HttpRouter +import com.twitter.inject.TwitterModule +import com.twitter.inject.thrift.modules.ThriftClientIdModule + +class TestHttpServer[C <: Controller: Manifest]( + serverName: String, + serverModules: TwitterModule* +) extends HttpServer { + override val name = serverName + + override val modules: Seq[Module] = + Seq(ThriftClientIdModule) ++ serverModules + + override def configureHttp(router: HttpRouter) { + router + .filter[CommonFilters] + .add[C] + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/thrift_server/GreeterThriftServer.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/TestThriftServer.scala similarity index 79% rename from inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/thrift_server/GreeterThriftServer.scala rename to inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/TestThriftServer.scala index 2af88ba451..b0dc2df67f 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/thrift_server/GreeterThriftServer.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/TestThriftServer.scala @@ -1,23 +1,19 @@ -package com.twitter.inject.thrift.filtered_integration.thrift_server +package com.twitter.inject.thrift.integration import com.twitter.finagle.{ListeningServer, ThriftMux} import com.twitter.inject.server.{PortUtils, TwitterServer} +import com.twitter.scrooge.ThriftService import com.twitter.util.Await -class GreeterThriftServer extends TwitterServer { - +class TestThriftServer(service: ThriftService) extends TwitterServer { private val thriftPortFlag = flag("thrift.port", ":0", "External Thrift server port") - /* Private Mutable State */ private var thriftServer: ListeningServer = _ /* Lifecycle */ - override def postWarmup() { super.postWarmup() - - thriftServer = ThriftMux.server.serveIface(thriftPortFlag(), injector.instance[GreeterImpl]) - + thriftServer = ThriftMux.server.serveIface(thriftPortFlag(), service) info("Thrift server started on port: " + thriftPort.get) } @@ -26,7 +22,5 @@ class GreeterThriftServer extends TwitterServer { } /* Overrides */ - override def thriftPort = Option(thriftServer) map PortUtils.getPort - } diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/ByeThriftClientFilter.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/ByeThriftClientFilter.scala similarity index 57% rename from inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/ByeThriftClientFilter.scala rename to inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/ByeThriftClientFilter.scala index ab00d03bc5..aecbcc08ff 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/ByeThriftClientFilter.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/ByeThriftClientFilter.scala @@ -1,11 +1,11 @@ -package com.twitter.inject.thrift.filtered_integration.http_server +package com.twitter.inject.thrift.integration.filtered -import com.twitter.finagle.{Filter, Service} +import com.twitter.finagle.Service import com.twitter.greeter.thriftscala.Greeter.Bye import com.twitter.inject.Logging import com.twitter.util.Future -class ByeThriftClientFilter extends Filter[Bye.Args, Bye.SuccessType, Bye.Args, Bye.SuccessType] with Logging { +class ByeThriftClientFilter extends ThriftClientFilter[Bye.Args, Bye.SuccessType] with Logging { def apply(request: Bye.Args, service: Service[Bye.Args, Bye.SuccessType]): Future[Bye.SuccessType] = { info("Bye called with name " + request.name) service(request) diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/GreeterFilteredThriftClientModule.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/GreeterFilteredThriftClientModule.scala similarity index 86% rename from inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/GreeterFilteredThriftClientModule.scala rename to inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/GreeterFilteredThriftClientModule.scala index a017aeb2b7..8d297ceab2 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/GreeterFilteredThriftClientModule.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/GreeterFilteredThriftClientModule.scala @@ -1,8 +1,9 @@ -package com.twitter.inject.thrift.filtered_integration.http_server +package com.twitter.inject.thrift.integration.filtered import com.twitter.greeter.thriftscala.Greeter.{Bye, Hello, Hi} import com.twitter.greeter.thriftscala.{Greeter, InvalidOperation} import com.twitter.inject.thrift.filters.ThriftClientFilterBuilder +import com.twitter.inject.thrift.integration.filters.MethodLoggingTypeAgnosticFilter import com.twitter.inject.thrift.modules.FilteredThriftClientModule import com.twitter.util.{Future, Return, Throw} import scala.util.control.NonFatal @@ -22,7 +23,7 @@ object GreeterFilteredThriftClientModule serviceIface.copy( hi = filter .method(Hi) - .withAgnosticFilter(new LogMethodTypeAgnosticFilter()) + .withAgnosticFilter(new MethodLoggingTypeAgnosticFilter()) .withMethodLatency .withConstantRetry( shouldRetry = { @@ -38,7 +39,7 @@ object GreeterFilteredThriftClientModule .andThen(serviceIface.hi), hello = filter .method(Hello) - .withAgnosticFilter(new LogMethodTypeAgnosticFilter()) + .withAgnosticFilter(new MethodLoggingTypeAgnosticFilter()) .withMethodLatency .withConstantRetry( shouldRetry = { @@ -54,7 +55,7 @@ object GreeterFilteredThriftClientModule .andThen(serviceIface.hello), bye = filter .method(Bye) - .withAgnosticFilter[LogMethodTypeAgnosticFilter] + .withAgnosticFilter[MethodLoggingTypeAgnosticFilter] .withMethodLatency .withExponentialRetry( shouldRetryResponse = PossiblyRetryableExceptions, diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/GreeterHttpController.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/GreeterHttpController.scala similarity index 88% rename from inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/GreeterHttpController.scala rename to inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/GreeterHttpController.scala index 66a2cd9a7f..5ac2d2fe30 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/GreeterHttpController.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/GreeterHttpController.scala @@ -1,4 +1,4 @@ -package com.twitter.inject.thrift.filtered_integration.http_server +package com.twitter.inject.thrift.integration.filtered import com.twitter.finagle.http.Request import com.twitter.finatra.http.Controller diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/thrift_server/GreeterImpl.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/GreeterThriftService.scala similarity index 88% rename from inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/thrift_server/GreeterImpl.scala rename to inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/GreeterThriftService.scala index 0b3ee442dc..391fdcb6fa 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/thrift_server/GreeterImpl.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/GreeterThriftService.scala @@ -1,4 +1,4 @@ -package com.twitter.inject.thrift.filtered_integration.thrift_server +package com.twitter.inject.thrift.integration.filtered import com.twitter.finagle.thrift.ClientId import com.twitter.greeter.thriftscala.{ByeOperation, ByeResponse, Greeter, InvalidOperation} @@ -8,7 +8,7 @@ import java.util.concurrent.atomic.AtomicInteger import javax.inject.Singleton @Singleton -class GreeterImpl extends Greeter[Future] with Logging { +class GreeterThriftService extends Greeter[Future] with Logging { private val hiNumCalled = new AtomicInteger(0) private val helloNumCalled = new AtomicInteger(0) @@ -58,6 +58,6 @@ class GreeterImpl extends Greeter[Future] with Logging { /* Private */ private def assertClientId(name: String): Unit = { - assert(ClientId.current.exists(_ == ClientId(name)), "Invalid Client ID: " + ClientId.current) + assert(ClientId.current.contains(ClientId(name)), "Invalid Client ID: " + ClientId.current) } } diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/HelloThriftClientFilter.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/HelloThriftClientFilter.scala similarity index 87% rename from inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/HelloThriftClientFilter.scala rename to inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/HelloThriftClientFilter.scala index ff15003ccb..8b1d3f1d60 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/HelloThriftClientFilter.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/HelloThriftClientFilter.scala @@ -1,4 +1,4 @@ -package com.twitter.inject.thrift.filtered_integration.http_server +package com.twitter.inject.thrift.integration.filtered import com.twitter.finagle.Service import com.twitter.greeter.thriftscala.Greeter.Hello diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/HiThriftClientFilter.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/HiThriftClientFilter.scala similarity index 86% rename from inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/HiThriftClientFilter.scala rename to inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/HiThriftClientFilter.scala index bc804d1698..018e1086f5 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/HiThriftClientFilter.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/HiThriftClientFilter.scala @@ -1,4 +1,4 @@ -package com.twitter.inject.thrift.filtered_integration.http_server +package com.twitter.inject.thrift.integration.filtered import com.twitter.finagle.Service import com.twitter.greeter.thriftscala.Greeter.Hi diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/ThriftClientFilter.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/ThriftClientFilter.scala similarity index 70% rename from inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/ThriftClientFilter.scala rename to inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/ThriftClientFilter.scala index 8436f6af8f..9092db19b6 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/ThriftClientFilter.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/ThriftClientFilter.scala @@ -1,4 +1,4 @@ -package com.twitter.inject.thrift.filtered_integration.http_server +package com.twitter.inject.thrift.integration.filtered import com.twitter.finagle.SimpleFilter import com.twitter.scrooge.ThriftStruct diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filters/HiLoggingTypeAgnosticFilter.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filters/HiLoggingTypeAgnosticFilter.scala new file mode 100644 index 0000000000..735ed65c5f --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filters/HiLoggingTypeAgnosticFilter.scala @@ -0,0 +1,23 @@ +package com.twitter.inject.thrift.integration.filters + +import com.twitter.finagle.{Filter, Service} +import com.twitter.greeter.thriftscala.Greeter.Hi +import com.twitter.inject.Logging +import com.twitter.scrooge +import com.twitter.util.Future + +class HiLoggingTypeAgnosticFilter extends Filter.TypeAgnostic with Logging { + + def toFilter[Req, Rep]: Filter[Req, Rep, Req, Rep] = new Filter[Req, Rep, Req, Rep] { + def apply(request: Req, service: Service[Req, Rep]): Future[Rep] = { + request match { + case _: scrooge.Request[_] => + info("Hi called with name " + request.asInstanceOf[scrooge.Request[Hi.Args]].args.name) + case _ => + info("Hi called with name " + request.asInstanceOf[Hi.Args].name) + } + + service(request) + } + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/LogMethodTypeAgnosticFilter.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filters/MethodLoggingTypeAgnosticFilter.scala similarity index 71% rename from inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/LogMethodTypeAgnosticFilter.scala rename to inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filters/MethodLoggingTypeAgnosticFilter.scala index 532d2bc071..11167a1863 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/filtered_integration/http_server/LogMethodTypeAgnosticFilter.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filters/MethodLoggingTypeAgnosticFilter.scala @@ -1,10 +1,10 @@ -package com.twitter.inject.thrift.filtered_integration.http_server +package com.twitter.inject.thrift.integration.filters import com.twitter.finagle.{Filter, Service} import com.twitter.inject.Logging import com.twitter.util.Future -class LogMethodTypeAgnosticFilter extends Filter.TypeAgnostic with Logging { +class MethodLoggingTypeAgnosticFilter extends Filter.TypeAgnostic with Logging { def toFilter[Req, Rep]: Filter[Req, Rep, Req, Rep] = new Filter[Req, Rep, Req, Rep] { def apply(request: Req, service: Service[Req, Rep]): Future[Rep] = { diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filters/SetTimesEchoTypeAgnosticFilter.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filters/SetTimesEchoTypeAgnosticFilter.scala new file mode 100644 index 0000000000..fd2715f93d --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filters/SetTimesEchoTypeAgnosticFilter.scala @@ -0,0 +1,23 @@ +package com.twitter.inject.thrift.integration.filters + +import com.twitter.finagle.{Filter, Service} +import com.twitter.inject.Logging +import com.twitter.scrooge +import com.twitter.util.Future +import com.twitter.test.thriftscala.EchoService.SetTimesToEcho + +class SetTimesEchoTypeAgnosticFilter extends Filter.TypeAgnostic with Logging { + + def toFilter[Req, Rep]: Filter[Req, Rep, Req, Rep] = new Filter[Req, Rep, Req, Rep] { + def apply(request: Req, service: Service[Req, Rep]): Future[Rep] = { + request match { + case _: scrooge.Request[_] => + info("SetTimesToEcho called with times " + request.asInstanceOf[scrooge.Request[SetTimesToEcho.Args]].args.times) + case _ => + info("SetTimesToEcho called with times " + request.asInstanceOf[SetTimesToEcho.Args].times) + } + + service(request) + } + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/http_server/EchoHttpController.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/http_server/EchoHttpController.scala deleted file mode 100644 index 45b4a3db90..0000000000 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/http_server/EchoHttpController.scala +++ /dev/null @@ -1,20 +0,0 @@ -package com.twitter.inject.thrift.integration.http_server - -import com.twitter.finagle.http.Request -import com.twitter.finatra.http.Controller -import com.twitter.test.thriftscala.EchoService -import com.twitter.util.Future -import javax.inject.Inject - -class EchoHttpController @Inject()(echoThriftService: EchoService[Future]) extends Controller { - - get("/echo") { request: Request => - val msg = request.params("msg") - echoThriftService.echo(msg) - } - - post("/config") { request: Request => - val timesToEcho = request.params("timesToEcho").toInt - echoThriftService.setTimesToEcho(timesToEcho) - } -} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/http_server/EchoHttpServer.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/http_server/EchoHttpServer.scala deleted file mode 100644 index 12540eaed6..0000000000 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/http_server/EchoHttpServer.scala +++ /dev/null @@ -1,16 +0,0 @@ -package com.twitter.inject.thrift.integration.http_server - -import com.twitter.finatra.http.HttpServer -import com.twitter.finatra.http.filters.CommonFilters -import com.twitter.finatra.http.routing.HttpRouter -import com.twitter.inject.thrift.modules.ThriftClientIdModule - -class EchoHttpServer extends HttpServer { - override val name = "echo-http-server" - - override val modules = Seq(ThriftClientIdModule, EchoThriftClientModule) - - override def configureHttp(router: HttpRouter) { - router.filter[CommonFilters].add[EchoHttpController] - } -} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/http_server/EchoThriftClientModule.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/http_server/EchoThriftClientModule.scala deleted file mode 100644 index 63feb83b11..0000000000 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/http_server/EchoThriftClientModule.scala +++ /dev/null @@ -1,10 +0,0 @@ -package com.twitter.inject.thrift.integration.http_server - -import com.twitter.inject.thrift.modules.ThriftClientModule -import com.twitter.test.thriftscala.EchoService -import com.twitter.util.Future - -object EchoThriftClientModule extends ThriftClientModule[EchoService[Future]] { - override val label = "echo-service" - override val dest = "flag!thrift-echo-service" -} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/EchoFilter.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/EchoFilter.scala new file mode 100644 index 0000000000..82833988d4 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/EchoFilter.scala @@ -0,0 +1,22 @@ +package com.twitter.inject.thrift.integration.inheritance + +import com.twitter.finagle.{Filter, Service} +import com.twitter.inject.Logging +import com.twitter.serviceA.thriftscala.ServiceA +import com.twitter.util.Future + +class EchoFilter + extends Filter[ + ServiceA.Echo.Args, + ServiceA.Echo.SuccessType, + ServiceA.Echo.Args, + ServiceA.Echo.SuccessType] + with Logging { + def apply( + request: ServiceA.Echo.Args, + service: Service[ServiceA.Echo.Args, String] + ): Future[ServiceA.Echo.SuccessType] = { + info("Echo called with name " + request.msg) + service(request) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/PingFilter.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/PingFilter.scala new file mode 100644 index 0000000000..6079295e54 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/PingFilter.scala @@ -0,0 +1,22 @@ +package com.twitter.inject.thrift.integration.inheritance + +import com.twitter.finagle.{Filter, Service} +import com.twitter.inject.Logging +import com.twitter.serviceB.thriftscala.ServiceB.Ping +import com.twitter.util.Future + +class PingFilter + extends Filter[ + Ping.Args, + Ping.SuccessType, + Ping.Args, + Ping.SuccessType] + with Logging { + def apply( + request: Ping.Args, + service: Service[Ping.Args, String] + ): Future[Ping.SuccessType] = { + info("Ping called") + service(request) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBHttpController.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBHttpController.scala new file mode 100644 index 0000000000..8471227679 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBHttpController.scala @@ -0,0 +1,17 @@ +package com.twitter.inject.thrift.integration.inheritance + +import com.twitter.finagle.http.Request +import com.twitter.finatra.http.Controller +import com.twitter.serviceB.thriftscala.ServiceB +import javax.inject.Inject + +class ServiceBHttpController @Inject()(serviceB: ServiceB.MethodPerEndpoint) extends Controller { + + get("/echo") { request: Request => + serviceB.echo(request.params("msg")) + } + + get("/ping") { _: Request => + serviceB.ping() + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBHttpServer.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBHttpServer.scala new file mode 100644 index 0000000000..4a8d9369db --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBHttpServer.scala @@ -0,0 +1,18 @@ +package com.twitter.inject.thrift.integration.inheritance + +import com.google.inject.Module +import com.twitter.finatra.http.HttpServer +import com.twitter.finatra.http.filters.CommonFilters +import com.twitter.finatra.http.routing.HttpRouter +import com.twitter.inject.thrift.modules.ThriftClientIdModule + +class ServiceBHttpServer extends HttpServer { + override val modules: Seq[Module] = + Seq(ThriftClientIdModule, ServiceBServicePerEndpointModule) + + override def configureHttp(router: HttpRouter) { + router + .filter[CommonFilters] + .add[ServiceBHttpController] + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBServicePerEndpointModule.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBServicePerEndpointModule.scala new file mode 100644 index 0000000000..942fed5b5d --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBServicePerEndpointModule.scala @@ -0,0 +1,35 @@ +package com.twitter.inject.thrift.integration.inheritance + +import com.google.inject.Module +import com.twitter.inject.thrift.ThriftMethodBuilderFactory +import com.twitter.inject.thrift.integration.filters.MethodLoggingTypeAgnosticFilter +import com.twitter.inject.thrift.modules.{ServicePerEndpointModule, ThriftClientIdModule} +import com.twitter.serviceA.thriftscala.ServiceA +import com.twitter.serviceB.thriftscala.ServiceB + +object ServiceBServicePerEndpointModule + extends ServicePerEndpointModule[ServiceB.ServicePerEndpoint, ServiceB.MethodPerEndpoint] { + + override val modules: Seq[Module] = Seq(ThriftClientIdModule) + + override val dest = "flag!serviceB-thrift-service" + override val label = "serviceB-thrift-client" + + override def configureServicePerEndpoint( + builder: ThriftMethodBuilderFactory[ServiceB.ServicePerEndpoint], + servicePerEndpoint: ServiceB.ServicePerEndpoint + ): ServiceB.ServicePerEndpoint = { + servicePerEndpoint + .withEcho( + builder.method[ServiceA.Echo.Args, ServiceA.Echo.SuccessType](ServiceA.Echo) + .withRetryForClassifier(PossiblyRetryableExceptions) + .withAgnosticFilter(new MethodLoggingTypeAgnosticFilter()) + .filtered[EchoFilter] + .service + ) + .withPing( + builder.method(ServiceB.Ping) + .filtered(new PingFilter) + .withRetryDisabled.service) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBThriftService.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBThriftService.scala new file mode 100644 index 0000000000..7fd0a7e612 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBThriftService.scala @@ -0,0 +1,27 @@ +package com.twitter.inject.thrift.integration.inheritance + +import com.twitter.finagle.Service +import com.twitter.inject.Logging +import com.twitter.inject.thrift.integration.AbstractThriftService +import com.twitter.util.Future +import com.twitter.serviceA.thriftscala.ServiceA +import com.twitter.serviceB.thriftscala.ServiceB + +class ServiceBThriftService( + clientId: String +) extends AbstractThriftService + with ServiceB.ServicePerEndpoint + with Logging { + + val ping: Service[ServiceB.Ping.Args, ServiceB.Ping.SuccessType] = + Service.mk { args: ServiceB.Ping.Args => + assertClientId(clientId) + Future.value("pong") + } + + val echo: Service[ServiceA.Echo.Args, ServiceA.Echo.SuccessType] = + Service.mk { args: ServiceA.Echo.Args => + assertClientId(clientId) + Future.value(args.msg) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/ByeFilter.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/ByeFilter.scala new file mode 100644 index 0000000000..a24a726661 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/ByeFilter.scala @@ -0,0 +1,25 @@ +package com.twitter.inject.thrift.integration.reqrepserviceperendpoint + +import com.twitter.finagle.{Filter, Service} +import com.twitter.greeter.thriftscala.Greeter.Bye +import com.twitter.inject.Logging +import com.twitter.scrooge +import com.twitter.util.Future +import javax.inject.Singleton + +@Singleton +class ByeFilter + extends Filter[ + scrooge.Request[Bye.Args], + scrooge.Response[Bye.SuccessType], + scrooge.Request[Bye.Args], + scrooge.Response[Bye.SuccessType]] + with Logging { + def apply( + request: scrooge.Request[Bye.Args], + service: Service[scrooge.Request[Bye.Args], scrooge.Response[Bye.SuccessType]] + ): Future[scrooge.Response[Bye.SuccessType]] = { + info("Bye called with name " + request.args.name) + service(request) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/ByeHeadersFilter.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/ByeHeadersFilter.scala new file mode 100644 index 0000000000..457b7d80b1 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/ByeHeadersFilter.scala @@ -0,0 +1,25 @@ +package com.twitter.inject.thrift.integration.reqrepserviceperendpoint + +import com.twitter.finagle.{Filter, Service} +import com.twitter.greeter.thriftscala.Greeter.Bye +import com.twitter.inject.Logging +import com.twitter.scrooge +import com.twitter.util.Future +import javax.inject.Singleton + +@Singleton +class ByeHeadersFilter( + requestHeaderKey: String +) extends Filter[ + scrooge.Request[Bye.Args], + scrooge.Response[Bye.SuccessType], + scrooge.Request[Bye.Args], + scrooge.Response[Bye.SuccessType]] + with Logging { + def apply( + request: scrooge.Request[Bye.Args], + service: Service[scrooge.Request[Bye.Args], scrooge.Response[Bye.SuccessType]] + ): Future[scrooge.Response[Bye.SuccessType]] = { + service(request.setHeader(requestHeaderKey, "bye")) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepServicePerEndpointModule.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepServicePerEndpointModule.scala new file mode 100644 index 0000000000..0339c65239 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepServicePerEndpointModule.scala @@ -0,0 +1,71 @@ +package com.twitter.inject.thrift.integration.reqrepserviceperendpoint + +import com.google.inject.Module +import com.twitter.finagle.service.{ReqRep, ResponseClass, ResponseClassifier} +import com.twitter.greeter.thriftscala.{Greeter, InvalidOperation} +import com.twitter.inject.thrift.ThriftMethodBuilderFactory +import com.twitter.inject.thrift.integration.filters.{HiLoggingTypeAgnosticFilter, MethodLoggingTypeAgnosticFilter} +import com.twitter.inject.thrift.modules.{ServicePerEndpointModule, ThriftClientIdModule} +import com.twitter.util.tunable.Tunable +import com.twitter.util.{Duration, Return, Throw} +import scala.util.control.NonFatal + +class GreeterReqRepServicePerEndpointModule( + requestHeaderKey: String, + timeoutPerRequestTunable: Tunable[Duration] +) extends ServicePerEndpointModule[Greeter.ReqRepServicePerEndpoint, Greeter.MethodPerEndpoint] { + + override val modules: Seq[Module] = Seq(ThriftClientIdModule) + + override val dest = "flag!greeter-thrift-service" + override val label = "greeter-thrift-client" + + override protected def configureServicePerEndpoint( + builder: ThriftMethodBuilderFactory[Greeter.ReqRepServicePerEndpoint], + servicePerEndpoint: Greeter.ReqRepServicePerEndpoint + ): Greeter.ReqRepServicePerEndpoint = { + servicePerEndpoint + .withHi( + builder + .method(Greeter.Hi) + .withTimeoutPerRequest(timeoutPerRequestTunable) + // method type-agnostic filter + .withAgnosticFilter(new HiLoggingTypeAgnosticFilter) + // method type-specific filter + .filtered(new HiHeadersFilter(requestHeaderKey)) + .withRetryForClassifier(PossiblyRetryableExceptions) + .service + ) + .withHello( + builder + .method(Greeter.Hello) + // method type-specific filter + .filtered(new HelloHeadersFilter(requestHeaderKey)) + // method type-specific filter + .filtered[HelloFilter] + .withRetryForClassifier(ByeResponseClassification) + .service + ) + .withBye( + builder + .method(Greeter.Bye) + // method type-specific filter + .filtered(new ByeHeadersFilter(requestHeaderKey)) + // method type-specific filter + .filtered[ByeFilter] + .withRetryForClassifier(PossiblyRetryableExceptions) + .service + ) + // global filter + .filtered(new MethodLoggingTypeAgnosticFilter()) + } + + private[this] val ByeResponseClassification: ResponseClassifier = + ResponseClassifier.named("ByeMethodCustomResponseClassification") { + case ReqRep(_, Return(result)) if result == "ERROR" => ResponseClass.RetryableFailure + case ReqRep(_, Return(_)) => ResponseClass.Success + case ReqRep(_, Throw(InvalidOperation(_))) => ResponseClass.RetryableFailure + case ReqRep(_, Throw(NonFatal(_))) => ResponseClass.RetryableFailure + case ReqRep(_, Throw(_)) => ResponseClass.NonRetryableFailure + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterThriftService.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterThriftService.scala new file mode 100644 index 0000000000..16d25a90aa --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterThriftService.scala @@ -0,0 +1,79 @@ +package com.twitter.inject.thrift.integration.reqrepserviceperendpoint + +import com.twitter.finagle.Service +import com.twitter.greeter.thriftscala.{ByeOperation, ByeResponse, Greeter, InvalidOperation} +import com.twitter.inject.Logging +import com.twitter.inject.thrift.integration.AbstractThriftService +import com.twitter.scrooge +import com.twitter.util.Future +import java.util.concurrent.atomic.AtomicInteger + +class GreeterThriftService( + clientId: String, + requestHeaderKey: String +) extends AbstractThriftService + with Greeter.ReqRepServicePerEndpoint + with Logging { + + private val hiNumCalled = new AtomicInteger(0) + private val helloNumCalled = new AtomicInteger(0) + private val byeNumCalled = new AtomicInteger(0) + + def hi: Service[scrooge.Request[Greeter.Hi.Args], scrooge.Response[Greeter.Hi.SuccessType]] = + Service.mk { request: scrooge.Request[Greeter.Hi.Args] => + if (requestHeaderKey.nonEmpty) assert(request.headers.contains(requestHeaderKey)) + val numCalled = hiNumCalled.incrementAndGet() + + val name = request.args.name + info(s"Hi called with message: " + name) + assertClientId(clientId) + + if (numCalled == 1) + Future.exception(new IllegalArgumentException) + else if (numCalled == 2) + Future.exception(new InvalidOperation(what = 123, why = "whoops")) + else if (numCalled == 3) + Future.value(scrooge.Response("ERROR")) + else + Future.value(scrooge.Response(s"Hi $name")) + } + + def hello: Service[ + scrooge.Request[Greeter.Hello.Args], + scrooge.Response[Greeter.Hello.SuccessType]] = + Service.mk { request: scrooge.Request[Greeter.Hello.Args] => + if (requestHeaderKey.nonEmpty) assert(request.headers.contains(requestHeaderKey)) + val numCalled = helloNumCalled.incrementAndGet() + + val name = request.args.name + info(s"Hello called with message: " + name) + assertClientId(clientId) + + if (numCalled == 1) + Future.exception(new IllegalArgumentException) + else if (numCalled == 2) + Future.exception(new InvalidOperation(what = 123, why = "whoops")) + else + Future.value( + scrooge.Response(s"Hello $name") + .setHeader("com.twitter.greeter.hello", "bar")) + } + + def bye: Service[scrooge.Request[Greeter.Bye.Args], scrooge.Response[Greeter.Bye.SuccessType]] = + Service.mk { request: scrooge.Request[Greeter.Bye.Args] => + if (requestHeaderKey.nonEmpty) assert(request.headers.contains(requestHeaderKey)) + val numCalled = byeNumCalled.incrementAndGet() + assertClientId(clientId) + + if (numCalled == 1) + Future.exception(new NullPointerException) + else if (numCalled == 2) + Future.exception(new ByeOperation(code = 456)) + else + Future.value( + scrooge.Response( + ByeResponse(code = 123, s"Bye ${request.args.name} of ${request.args.age} years!") + ) + ) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/HelloFilter.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/HelloFilter.scala new file mode 100644 index 0000000000..7effd00958 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/HelloFilter.scala @@ -0,0 +1,27 @@ +package com.twitter.inject.thrift.integration.reqrepserviceperendpoint + +import com.twitter.finagle.{Filter, Service} +import com.twitter.greeter.thriftscala.Greeter.Hello +import com.twitter.inject.Logging +import com.twitter.scrooge +import com.twitter.util.Future +import javax.inject.Singleton + +@Singleton +class HelloFilter + extends Filter[ + scrooge.Request[Hello.Args], + scrooge.Response[Hello.SuccessType], + scrooge.Request[Hello.Args], + scrooge.Response[Hello.SuccessType]] + with Logging { + def apply( + request: scrooge.Request[Hello.Args], + service: Service[scrooge.Request[Hello.Args], scrooge.Response[Hello.SuccessType]] + ): Future[scrooge.Response[Hello.SuccessType]] = { + info("Hello called with name " + request.args.name) + service(request).onSuccess { response => + info(response) + } + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/HelloHeadersFilter.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/HelloHeadersFilter.scala new file mode 100644 index 0000000000..78c9c7b6d5 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/HelloHeadersFilter.scala @@ -0,0 +1,25 @@ +package com.twitter.inject.thrift.integration.reqrepserviceperendpoint + +import com.twitter.finagle.{Filter, Service} +import com.twitter.greeter.thriftscala.Greeter.Hello +import com.twitter.inject.Logging +import com.twitter.scrooge +import com.twitter.util.Future +import javax.inject.Singleton + +@Singleton +class HelloHeadersFilter( + requestHeaderKey: String +) extends Filter[ + scrooge.Request[Hello.Args], + scrooge.Response[Hello.SuccessType], + scrooge.Request[Hello.Args], + scrooge.Response[Hello.SuccessType]] + with Logging { + def apply( + request: scrooge.Request[Hello.Args], + service: Service[scrooge.Request[Hello.Args], scrooge.Response[Hello.SuccessType]] + ): Future[scrooge.Response[Hello.SuccessType]] = { + service(request.setHeader(requestHeaderKey, "hello")) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/HiHeadersFilter.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/HiHeadersFilter.scala new file mode 100644 index 0000000000..4f7c98fb4f --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/HiHeadersFilter.scala @@ -0,0 +1,23 @@ +package com.twitter.inject.thrift.integration.reqrepserviceperendpoint + +import com.twitter.finagle.{Filter, Service} +import com.twitter.greeter.thriftscala.Greeter.Hi +import com.twitter.inject.Logging +import com.twitter.scrooge +import com.twitter.util.Future + +class HiHeadersFilter( + requestHeaderKey: String +) extends Filter[ + scrooge.Request[Hi.Args], + scrooge.Response[Hi.SuccessType], + scrooge.Request[Hi.Args], + scrooge.Response[Hi.SuccessType]] + with Logging { + def apply( + request: scrooge.Request[Hi.Args], + service: Service[scrooge.Request[Hi.Args], scrooge.Response[Hi.SuccessType]] + ): Future[scrooge.Response[Hi.SuccessType]] = { + service(request.setHeader(requestHeaderKey, "hi")) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/ReqRepServicePerEndpointHttpController.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/ReqRepServicePerEndpointHttpController.scala new file mode 100644 index 0000000000..b2b5bc6328 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/ReqRepServicePerEndpointHttpController.scala @@ -0,0 +1,30 @@ +package com.twitter.inject.thrift.integration.reqrepserviceperendpoint + +import com.twitter.finagle.http.Request +import com.twitter.finatra.http.Controller +import com.twitter.greeter.thriftscala.Greeter +import com.twitter.greeter.thriftscala.Greeter.{Bye, Hello, Hi} +import com.twitter.scrooge +import javax.inject.Inject + +class ReqRepServicePerEndpointHttpController @Inject()( + greeter: Greeter.ReqRepServicePerEndpoint +) extends Controller { + + get("/hi") { request: Request => + greeter.hi(scrooge.Request(Hi.Args(name = request.params("name")))).map(_.value) + } + + get("/hello") { request: Request => + greeter.hello(scrooge.Request(Hello.Args(name = request.params("name")))) + } + + get("/bye") { request: Request => + greeter + .bye( + scrooge.Request(Bye.Args(name = request.params("name"), age = request.getIntParam("age"))) + ) + .map(_.value.msg) + } + +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/ByeFilter.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/ByeFilter.scala new file mode 100644 index 0000000000..07ab92bb83 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/ByeFilter.scala @@ -0,0 +1,24 @@ +package com.twitter.inject.thrift.integration.serviceperendpoint + +import com.twitter.finagle.{Filter, Service} +import com.twitter.greeter.thriftscala.Greeter.Bye +import com.twitter.inject.Logging +import com.twitter.util.Future +import javax.inject.Singleton + +@Singleton +class ByeFilter + extends Filter[ + Bye.Args, + Bye.SuccessType, + Bye.Args, + Bye.SuccessType] + with Logging { + def apply( + request: Bye.Args, + service: Service[Bye.Args, Bye.SuccessType] + ): Future[Bye.SuccessType] = { + info("Bye called with name " + request.name) + service(request) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoFilter.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoFilter.scala new file mode 100644 index 0000000000..89ab708987 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoFilter.scala @@ -0,0 +1,21 @@ +package com.twitter.inject.thrift.integration.serviceperendpoint + +import com.twitter.finagle.{Filter, Service} +import com.twitter.inject.Logging +import com.twitter.test.thriftscala.EchoService.Echo + +class EchoFilter + extends Filter[ + Echo.Args, + Echo.SuccessType, + Echo.Args, + Echo.SuccessType] + with Logging { + override def apply( + request: Echo.Args, + service: Service[Echo.Args, String] + ) = { + info("Echo called with msg " + request.msg) + service(request) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoServicePerEndpointModule.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoServicePerEndpointModule.scala new file mode 100644 index 0000000000..051d7badd3 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoServicePerEndpointModule.scala @@ -0,0 +1,38 @@ +package com.twitter.inject.thrift.integration.serviceperendpoint + +import com.google.inject.Module +import com.twitter.inject.thrift.ThriftMethodBuilderFactory +import com.twitter.inject.thrift.integration.filters.{MethodLoggingTypeAgnosticFilter, SetTimesEchoTypeAgnosticFilter} +import com.twitter.inject.thrift.modules.{ServicePerEndpointModule, ThriftClientIdModule} +import com.twitter.test.thriftscala.EchoService + +object EchoServicePerEndpointModule + extends ServicePerEndpointModule[EchoService.ServicePerEndpoint, EchoService.MethodPerEndpoint] { + + override val modules: Seq[Module] = Seq(ThriftClientIdModule) + + override val dest = "flag!echo-thrift-service" + override val label = "echo-thrift-client" + + override def configureServicePerEndpoint( + builder: ThriftMethodBuilderFactory[EchoService.ServicePerEndpoint], + servicePerEndpoint: EchoService.ServicePerEndpoint + ): EchoService.ServicePerEndpoint = { + + servicePerEndpoint + .withEcho( + builder.method[EchoService.Echo.Args, EchoService.Echo.SuccessType](EchoService.Echo) + // method type-specific filter + .filtered[EchoFilter] + .withRetryForClassifier(PossiblyRetryableExceptions) + .service) + .withSetTimesToEcho( + builder.method(EchoService.SetTimesToEcho) + // method type-agnostic filter + .withAgnosticFilter(new SetTimesEchoTypeAgnosticFilter()) + .withRetryForClassifier(PossiblyRetryableExceptions) + .service) + // global (type-agnostic) filter + .filtered(new MethodLoggingTypeAgnosticFilter()) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoThriftService.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoThriftService.scala new file mode 100644 index 0000000000..e49ea57a3f --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoThriftService.scala @@ -0,0 +1,33 @@ +package com.twitter.inject.thrift.integration.serviceperendpoint + +import com.twitter.finagle.Service +import com.twitter.inject.Logging +import com.twitter.inject.thrift.integration.AbstractThriftService +import com.twitter.test.thriftscala.EchoService +import com.twitter.test.thriftscala.EchoService.{Echo, SetTimesToEcho} +import com.twitter.util.Future +import java.util.concurrent.atomic.AtomicInteger + +class EchoThriftService( + clientId: String +) extends AbstractThriftService + with EchoService.ServicePerEndpoint + with Logging { + + private val timesToEcho = new AtomicInteger(1) + + /* Public */ + + def echo: Service[Echo.Args, Echo.SuccessType] = Service.mk { args: Echo.Args => + info("echo " + args.msg) + assertClientId(clientId) + Future.value(args.msg * timesToEcho.get) + } + + def setTimesToEcho: Service[SetTimesToEcho.Args, SetTimesToEcho.SuccessType] = Service.mk { args: SetTimesToEcho.Args => + info("setTimesToEcho " + args.times) + assertClientId(clientId) + timesToEcho.set(args.times) //mutation + Future.value(args.times) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterServicePerEndpointModule.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterServicePerEndpointModule.scala new file mode 100644 index 0000000000..c7679f8a60 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterServicePerEndpointModule.scala @@ -0,0 +1,57 @@ +package com.twitter.inject.thrift.integration.serviceperendpoint + +import com.google.inject.Module +import com.twitter.finagle.service.{ReqRep, ResponseClass, ResponseClassifier} +import com.twitter.greeter.thriftscala.Greeter.Bye +import com.twitter.greeter.thriftscala.{Greeter, InvalidOperation} +import com.twitter.inject.thrift.ThriftMethodBuilderFactory +import com.twitter.inject.thrift.integration.filters.{HiLoggingTypeAgnosticFilter, MethodLoggingTypeAgnosticFilter} +import com.twitter.inject.thrift.modules.{ServicePerEndpointModule, ThriftClientIdModule} +import com.twitter.util.{Return, Throw} +import scala.util.control.NonFatal + +object GreeterServicePerEndpointModule + extends ServicePerEndpointModule[Greeter.ServicePerEndpoint, Greeter.MethodPerEndpoint] { + + override val modules: Seq[Module] = Seq(ThriftClientIdModule) + + override val dest = "flag!greeter-thrift-service" + override val label = "greeter-thrift-client" + + override def configureServicePerEndpoint( + builder: ThriftMethodBuilderFactory[Greeter.ServicePerEndpoint], + servicePerEndpoint: Greeter.ServicePerEndpoint + ): Greeter.ServicePerEndpoint = { + + servicePerEndpoint + .withHi( + builder.method[Greeter.Hi.Args, Greeter.Hi.SuccessType](Greeter.Hi) + // method type-agnostic filter + .withAgnosticFilter[HiLoggingTypeAgnosticFilter] + .withRetryForClassifier(PossiblyRetryableExceptions) + .service) + .withHello( + builder.method(Greeter.Hello) + // method type-specific filter + .filtered(new HelloFilter) + .withRetryForClassifier(ByeResponseClassification) + .service) + .withBye( + builder.method[Bye.Args, Bye.SuccessType](Greeter.Bye) + // method type-specific filter + .filtered[ByeFilter] + .withRetryForClassifier(PossiblyRetryableExceptions) + .service) + // global (type-agnostic) filter + .filtered(new MethodLoggingTypeAgnosticFilter()) + } + + private[this] val ByeResponseClassification: ResponseClassifier = + ResponseClassifier.named("ByeMethodCustomResponseClassification") { + case ReqRep(_, Return(result)) if result == "ERROR" => ResponseClass.RetryableFailure + case ReqRep(_, Return(_)) => ResponseClass.Success + case ReqRep(_, Throw(InvalidOperation(_))) => ResponseClass.RetryableFailure + case ReqRep(_, Throw(NonFatal(_))) => ResponseClass.RetryableFailure + case ReqRep(_, Throw(_)) => ResponseClass.NonRetryableFailure + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterThriftService.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterThriftService.scala new file mode 100644 index 0000000000..936bf88b74 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterThriftService.scala @@ -0,0 +1,65 @@ +package com.twitter.inject.thrift.integration.serviceperendpoint + +import com.twitter.finagle.Service +import com.twitter.greeter.thriftscala.{ByeOperation, ByeResponse, Greeter, InvalidOperation} +import com.twitter.inject.Logging +import com.twitter.inject.thrift.integration.AbstractThriftService +import com.twitter.util.Future +import java.util.concurrent.atomic.AtomicInteger + +class GreeterThriftService( + clientId: String +) extends AbstractThriftService + with Greeter.ServicePerEndpoint + with Logging { + + private val hiNumCalled = new AtomicInteger(0) + private val helloNumCalled = new AtomicInteger(0) + private val byeNumCalled = new AtomicInteger(0) + + def hi: Service[Greeter.Hi.Args, Greeter.Hi.SuccessType] = Service.mk { args: Greeter.Hi.Args => + val numCalled = hiNumCalled.incrementAndGet() + + val name = args.name + info(s"Hi called with message: " + name) + assertClientId(clientId) + + if (numCalled == 1) + Future.exception(new IllegalArgumentException) + else if (numCalled == 2) + Future.exception(new InvalidOperation(what = 123, why = "whoops")) + else if (numCalled == 3) + Future.value("ERROR") + else + Future.value(s"Hi $name") + } + + def hello: Service[Greeter.Hello.Args, Greeter.Hello.SuccessType] = Service.mk { + args: Greeter.Hello.Args => + val numCalled = helloNumCalled.incrementAndGet() + + val name = args.name + info(s"Hello called with message: " + name) + assertClientId(clientId) + + if (numCalled == 1) + Future.exception(new IllegalArgumentException) + else if (numCalled == 2) + Future.exception(new InvalidOperation(what = 123, why = "whoops")) + else + Future.value(s"Hello $name") + } + + def bye: Service[Greeter.Bye.Args, Greeter.Bye.SuccessType] = Service.mk { + args: Greeter.Bye.Args => + val numCalled = byeNumCalled.incrementAndGet() + assertClientId(clientId) + + if (numCalled == 1) + Future.exception(new NullPointerException) + else if (numCalled == 2) + Future.exception(new ByeOperation(code = 456)) + else + Future.value(ByeResponse(code = 123, s"Bye ${args.name} of ${args.age} years!")) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/HelloFilter.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/HelloFilter.scala new file mode 100644 index 0000000000..e67d7a86b5 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/HelloFilter.scala @@ -0,0 +1,24 @@ +package com.twitter.inject.thrift.integration.serviceperendpoint + +import com.twitter.finagle.{Filter, Service} +import com.twitter.greeter.thriftscala.Greeter.Hello +import com.twitter.inject.Logging +import com.twitter.util.Future +import javax.inject.Singleton + +@Singleton +class HelloFilter + extends Filter[ + Hello.Args, + Hello.SuccessType, + Hello.Args, + Hello.SuccessType] + with Logging { + def apply( + request: Hello.Args, + service: Service[Hello.Args, Hello.SuccessType] + ): Future[Hello.SuccessType] = { + info("Hello called with name " + request.name) + service(request) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/ServicePerEndpointHttpController.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/ServicePerEndpointHttpController.scala new file mode 100644 index 0000000000..08bf0407b4 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/ServicePerEndpointHttpController.scala @@ -0,0 +1,37 @@ +package com.twitter.inject.thrift.integration.serviceperendpoint + +import com.twitter.finagle.http.Request +import com.twitter.finatra.http.Controller +import com.twitter.greeter.thriftscala.Greeter +import com.twitter.greeter.thriftscala.Greeter.{Bye, Hello, Hi} +import com.twitter.test.thriftscala.EchoService +import com.twitter.test.thriftscala.EchoService.{Echo, SetTimesToEcho} +import javax.inject.Inject + +class ServicePerEndpointHttpController @Inject()( + greeter: Greeter.ServicePerEndpoint, + echo: EchoService.ServicePerEndpoint) + extends Controller { + + get("/hi") { request: Request => + greeter.hi(Hi.Args(name = request.params("name"))) + } + + get("/hello") { request: Request => + greeter.hello(Hello.Args(name = request.params("name"))) + } + + get("/bye") { request: Request => + greeter.bye(Bye.Args(name = request.params("name"), age = request.getIntParam("age"))).map(_.msg) + } + + get("/echo") { request: Request => + val msg = request.params("msg") + echo.echo(Echo.Args(msg = msg)) + } + + post("/config") { request: Request => + val timesToEcho = request.params("timesToEcho").toInt + echo.setTimesToEcho(SetTimesToEcho.Args(times = timesToEcho)) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/thrift_server/EchoThriftServer.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/thrift_server/EchoThriftServer.scala deleted file mode 100644 index f10d14f004..0000000000 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/thrift_server/EchoThriftServer.scala +++ /dev/null @@ -1,38 +0,0 @@ -package com.twitter.inject.thrift.integration.thrift_server - -import com.twitter.conversions.time._ -import com.twitter.finagle.{ListeningServer, ThriftMux} -import com.twitter.inject.server.{PortUtils, TwitterServer} -import com.twitter.util.Await - -class EchoThriftServer extends TwitterServer { - - private val thriftPortFlag = flag("thrift.port", ":0", "External Thrift server port") - private val thriftShutdownTimeout = flag( - "thrift.shutdown.time", - 1.minute, - "Maximum amount of time to wait for pending requests to complete on shutdown" - ) - - /* Private Mutable State */ - private var thriftServer: ListeningServer = _ - - /* Lifecycle */ - - override def postWarmup() { - super.postWarmup() - - thriftServer = ThriftMux.server.serveIface(thriftPortFlag(), injector.instance[MyEchoService]) - - info("Thrift server started on port: " + thriftPort.get) - } - - onExit { - Await.result(thriftServer.close(thriftShutdownTimeout().fromNow)) - } - - /* Overrides */ - - override def thriftPort = Option(thriftServer) map PortUtils.getPort - -} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/thrift_server/MyEchoService.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/thrift_server/MyEchoService.scala deleted file mode 100644 index 791ee7a6cb..0000000000 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/thrift_server/MyEchoService.scala +++ /dev/null @@ -1,35 +0,0 @@ -package com.twitter.inject.thrift.integration.thrift_server - -import com.twitter.finagle.thrift.ClientId -import com.twitter.inject.Logging -import com.twitter.test.thriftscala.EchoService -import com.twitter.util.Future -import java.util.concurrent.atomic.AtomicInteger -import javax.inject.Singleton - -@Singleton -class MyEchoService extends EchoService[Future] with Logging { - - private val timesToEcho = new AtomicInteger(1) - - /* Public */ - - override def echo(msg: String): Future[String] = { - info("echo " + msg) - assertClientId("echo-http-service") - Future.value(msg * timesToEcho.get) - } - - override def setTimesToEcho(times: Int): Future[Int] = { - info("setTimesToEcho " + times) - assertClientId("echo-http-service") - timesToEcho.set(times) //mutation - Future(times) - } - - /* Private */ - - private def assertClientId(name: String): Unit = { - assert(ClientId.current.exists(_ == ClientId(name)), "Invalid Client ID: " + ClientId.current) - } -} diff --git a/inject/inject-thrift-client/src/test/thrift/serviceA.thrift b/inject/inject-thrift-client/src/test/thrift/serviceA.thrift new file mode 100644 index 0000000000..cbed1f9bf2 --- /dev/null +++ b/inject/inject-thrift-client/src/test/thrift/serviceA.thrift @@ -0,0 +1,7 @@ +namespace java com.twitter.serviceA.thriftjava +#@namespace scala com.twitter.serviceA.thriftscala +namespace rb ServiceA + +service ServiceA { + string echo(string msg) +} \ No newline at end of file diff --git a/inject/inject-thrift-client/src/test/thrift/serviceB.thrift b/inject/inject-thrift-client/src/test/thrift/serviceB.thrift new file mode 100644 index 0000000000..284d1e4136 --- /dev/null +++ b/inject/inject-thrift-client/src/test/thrift/serviceB.thrift @@ -0,0 +1,9 @@ +namespace java com.twitter.serviceB.thriftjava +#@namespace scala com.twitter.serviceB.thriftscala +namespace rb ServiceB + +include "serviceA.thrift" + +service ServiceB extends serviceA.ServiceA { + string ping() +} \ No newline at end of file From 9d3d965326493a71e8f6871a542ac9ab9e97f378 Mon Sep 17 00:00:00 2001 From: Christopher Coco Date: Mon, 22 Jan 2018 22:16:40 +0000 Subject: [PATCH 03/12] inject-thrift-client: Light test cleanup Summary: Problem/Solution Update test comments for clarity. JIRA Issues: CSL-5842 Differential Revision: https://phabricator.twitter.biz/D130041 --- .../InheritanceServicePerEndpointModuleFeatureTest.scala | 3 +-- .../inheritance/ServiceBServicePerEndpointModule.scala | 6 +++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/InheritanceServicePerEndpointModuleFeatureTest.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/InheritanceServicePerEndpointModuleFeatureTest.scala index ae11f54c66..5c8587b179 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/InheritanceServicePerEndpointModuleFeatureTest.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/InheritanceServicePerEndpointModuleFeatureTest.scala @@ -69,8 +69,7 @@ class InheritanceServicePerEndpointModuleFeatureTest server.assertCounter("clnt/serviceB-thrift-client/ServiceB/ping/failures", 0) /* assert MethodBuilder stats exist */ server.getStat("clnt/serviceB-thrift-client/ping/logical/request_latency_ms") should not be Seq() - // TODO: CSL-5842 - // server.getStat("clnt/serviceB-thrift-client/ping/retries") should be(Seq(0.0)) + // retries are disabled, thus no "clnt/serviceB-thrift-client/ping/retries" stat /* assert MethodBuilder counters */ server.assertCounter("clnt/serviceB-thrift-client/ping/logical/requests", 1) server.assertCounter("clnt/serviceB-thrift-client/ping/logical/success", 1) diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBServicePerEndpointModule.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBServicePerEndpointModule.scala index 942fed5b5d..6d43c20f41 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBServicePerEndpointModule.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBServicePerEndpointModule.scala @@ -25,11 +25,11 @@ object ServiceBServicePerEndpointModule .withRetryForClassifier(PossiblyRetryableExceptions) .withAgnosticFilter(new MethodLoggingTypeAgnosticFilter()) .filtered[EchoFilter] - .service - ) + .service) .withPing( builder.method(ServiceB.Ping) .filtered(new PingFilter) - .withRetryDisabled.service) + .withRetryDisabled + .service) } } From 7868964d223656f4e2b2ab738c835f45e30add76 Mon Sep 17 00:00:00 2001 From: Jillian Crossley Date: Tue, 23 Jan 2018 00:38:58 +0000 Subject: [PATCH 04/12] finatra: Add idempotent/nonIdempotent methods on MethodBuilder to ThriftMethodBuilder Summary: Problem We'd like to provide the same MethodBuilder idempotent/nonIdempotent configuration functionality in Finatra's ThriftMethodBuilder as in Finagle. Solution Add idempotent/nonIdempotent methods to ThriftMethodBuilder. JIRA Issues: CSL-5853 Differential Revision: https://phabricator.twitter.biz/D129959 --- CHANGELOG.md | 4 ++++ .../inject/thrift/ThriftMethodBuilder.scala | 24 +++++++++++++++++++ .../ServiceBServicePerEndpointModule.scala | 1 + ...reeterReqRepServicePerEndpointModule.scala | 2 ++ 4 files changed, 31 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1045e631fe..2ecdee91f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ All notable changes to this project will be documented in this file. Note that ` ### Added +* inject-thrift-client: Add `idempotent` and `nonIdempotent` methods to + `c.t.inject.thrift.ThriftMethodBuilder`, which can be used to configure retries and the sending of + backup requests. ``PHAB_ID=D129959`` + * inject-thrift-client: Add `c.t.inject.thrift.modules.ServicePerEndpointModule` for building ThriftMux clients using the `thriftmux.MethodBuilder`. ``PHAB_ID=D128196`` diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/ThriftMethodBuilder.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/ThriftMethodBuilder.scala index 8ba376da02..887591a61c 100644 --- a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/ThriftMethodBuilder.scala +++ b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/ThriftMethodBuilder.scala @@ -122,6 +122,30 @@ final class ThriftMethodBuilder[ServicePerEndpoint <: Filterable[ServicePerEndpo this } + /** + * @see [[com.twitter.finagle.thriftmux.MethodBuilder.idempotent(maxExtraLoad: Double)]] + */ + def idempotent(maxExtraLoad: Double): this.type = { + methodBuilder = methodBuilder.idempotent(maxExtraLoad) + this + } + + /** + * @see [[com.twitter.finagle.thriftmux.MethodBuilder.idempotent(maxExtraLoad: Tunable[Double])]] + */ + def idempotent(maxExtraLoad: Tunable[Double]): this.type = { + methodBuilder = methodBuilder.idempotent(maxExtraLoad) + this + } + + /** + * @see [[com.twitter.finagle.thriftmux.MethodBuilder.nonIdempotent]] + */ + def nonIdempotent: this.type = { + methodBuilder = methodBuilder.nonIdempotent + this + } + /** * Install a [[com.twitter.finagle.Filter]]. This filter will be added to the end of the filter chain. That is, this * filter will be invoked AFTER any other installed filter on a request [[Req]] and thus BEFORE any other installed diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBServicePerEndpointModule.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBServicePerEndpointModule.scala index 6d43c20f41..5568c26a4c 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBServicePerEndpointModule.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBServicePerEndpointModule.scala @@ -29,6 +29,7 @@ object ServiceBServicePerEndpointModule .withPing( builder.method(ServiceB.Ping) .filtered(new PingFilter) + .nonIdempotent .withRetryDisabled .service) } diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepServicePerEndpointModule.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepServicePerEndpointModule.scala index 0339c65239..0b982c0a31 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepServicePerEndpointModule.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepServicePerEndpointModule.scala @@ -1,6 +1,7 @@ package com.twitter.inject.thrift.integration.reqrepserviceperendpoint import com.google.inject.Module +import com.twitter.conversions.percent._ import com.twitter.finagle.service.{ReqRep, ResponseClass, ResponseClassifier} import com.twitter.greeter.thriftscala.{Greeter, InvalidOperation} import com.twitter.inject.thrift.ThriftMethodBuilderFactory @@ -34,6 +35,7 @@ class GreeterReqRepServicePerEndpointModule( // method type-specific filter .filtered(new HiHeadersFilter(requestHeaderKey)) .withRetryForClassifier(PossiblyRetryableExceptions) + .idempotent(1.percent) .service ) .withHello( From 791b75d7848be87c07cb501b4272f5fc4e78370a Mon Sep 17 00:00:00 2001 From: Yufan Gong Date: Wed, 24 Jan 2018 02:11:31 +0000 Subject: [PATCH 05/12] scrooge: Add asClosable.close method for thrift rich client Summary: Problem: Request from https://github.com/twitter/finagle/issues/572, it is currently not possible to close clients instantiated via the MethodPerEndpoint interface. ``` val client: LoggerService.MethodPerEndpoint = Thrift.client.build[LoggerService.MethodPerEndpoint]("localhost:1234") ``` We want a method to close the connection. e.g.: ``` client.asClosable.close() ``` Solution: Update scrooge service generators to provide `asClosable` method which returns a `Closable`, and automatically disable `asClosable` method when detected a user-defined one. JIRA Issues: CSL-3618 Differential Revision: https://phabricator.twitter.biz/D129645 --- ...rythingThriftClientModuleFeatureTest.scala | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingThriftClientModuleFeatureTest.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingThriftClientModuleFeatureTest.scala index f6d75cecff..72c17aa473 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingThriftClientModuleFeatureTest.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingThriftClientModuleFeatureTest.scala @@ -4,7 +4,7 @@ import com.twitter.conversions.time._ import com.twitter.finagle.http.Request import com.twitter.finagle.http.Status._ import com.twitter.finagle.thrift.ClientId -import com.twitter.finagle.{ListeningServer, ThriftMux} +import com.twitter.finagle.{ListeningServer, ServiceClosedException, ThriftMux} import com.twitter.finatra.http.filters.CommonFilters import com.twitter.finatra.http.routing.HttpRouter import com.twitter.finatra.http.{Controller, EmbeddedHttpServer, HttpServer, HttpTest} @@ -103,6 +103,9 @@ object DoEverythingThriftClientModuleFeatureTest { } class DoEverythingThriftClientModuleFeatureTest extends Test with HttpTest { + + private val clientIdString = "echo-http-service" + val thriftServer = new EmbeddedThriftServer( twitterServer = new EchoThriftServer) @@ -116,7 +119,7 @@ class DoEverythingThriftClientModuleFeatureTest extends Test with HttpTest { } }, args = Seq( - "-thrift.clientId=echo-http-service", + s"-thrift.clientId=$clientIdString", resolverMap("thrift-echo-service" -> thriftServer.thriftHostAndPort) ) ) @@ -183,4 +186,19 @@ class DoEverythingThriftClientModuleFeatureTest extends Test with HttpTest { httpServer2.assertStat("route/config/POST/response_size", Seq(1, 1)) httpServer2.assertStat("route/echo/GET/response_size", Seq(9)) } + + test("EchoThriftServer#echo 3 times") { + val thriftClient = thriftServer + .thriftClient[EchoService[Future]](clientIdString) + + Await.result(thriftClient.setTimesToEcho(2), 5.seconds) + Await.result(thriftClient.setTimesToEcho(3), 5.seconds) + + assert(Await.result(thriftClient.echo("Bob"), 5.seconds) == "BobBobBob") + + Await.result(thriftClient.asClosable.close(), 5.seconds) + intercept[ServiceClosedException] { + Await.result(thriftClient.echo("Bob"), 5.seconds) + } + } } From 72664be4439da4425dfe63fa325f4c1ebbc5bf4b Mon Sep 17 00:00:00 2001 From: Andreea Dimitriu Date: Wed, 24 Jan 2018 09:51:50 +0000 Subject: [PATCH 06/12] finatra-inject: Updated the thrift client to accept a tunable as a timeout parameter Summary: Problem As Tunables users, we would like to be able to pass in a tunable timeout to our thrift clients Solution Update the thrift client to accept a tunable as a timeout parameter Depends on D128503 Differential Revision: https://phabricator.twitter.biz/D128506 --- CHANGELOG.md | 3 ++ .../filters/ThriftClientFilterChain.scala | 41 +++++++++++++++++++ .../GreeterFilteredThriftClientModule.scala | 7 +++- 3 files changed, 50 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ecdee91f0..55cdf76af1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ All notable changes to this project will be documented in this file. Note that ` ### Added +* inject-thrift-client: Add methods to `c.t.inject.thrift.filters.ThriftClientFilterChain` to allow + Tunable timeouts and request timeouts. ``PHAB_ID=D128506`` + * inject-thrift-client: Add `idempotent` and `nonIdempotent` methods to `c.t.inject.thrift.ThriftMethodBuilder`, which can be used to configure retries and the sending of backup requests. ``PHAB_ID=D129959`` diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterChain.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterChain.scala index 2145f8d4e3..a1885dd481 100644 --- a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterChain.scala +++ b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterChain.scala @@ -24,6 +24,7 @@ import com.twitter.inject.thrift.utils.ThriftMethodUtils import com.twitter.inject.utils.ExceptionUtils._ import com.twitter.inject.{Injector, Logging} import com.twitter.scrooge.{ThriftMethod, ThriftStruct} +import com.twitter.util.tunable.Tunable import com.twitter.util.{Timer, Try, Duration => TwitterDuration} import java.util.concurrent.TimeUnit import org.joda.time.Duration @@ -333,6 +334,26 @@ class ThriftClientFilterChain[Req <: ThriftStruct, Rep]( this } + /** + * Install a [[com.twitter.finagle.service.TimeoutFilter]] configuration with the given tunable timeout. + * This filter will be "above" any configured retry filter and thus includes retries. + * + * @param duration the Tunable[Duration] ([[org.joda.time.Duration]]) timeout to apply to requests through the filter. + * @see [[com.twitter.finagle.service.TimeoutFilter]] + * @return [[ThriftClientFilterChain]] + */ + def withTimeout(duration: Tunable[Duration]): ThriftClientFilterChain[Req, Rep] = { + val exceptionFn = (duration: TwitterDuration) => + new GlobalRequestTimeoutException(duration * timeoutMultiplier) + + timeoutFilter = new TimeoutFilter[Req, Rep]( + duration.map(d => d.toTwitterDuration), + exceptionFn, + DefaultTimer + ) + this + } + /** * Install a [[com.twitter.finagle.service.TimeoutFilter]] configuration with the given [[org.joda.time.Duration]] timeout. * This filter will always be "below" any configured retry filter and thus does NOT include retries. @@ -352,6 +373,26 @@ class ThriftClientFilterChain[Req <: ThriftStruct, Rep]( this } + /** + * Install a [[com.twitter.finagle.service.TimeoutFilter]] configuration with the given tunable timeout. + * This filter will always be "below" any configured retry filter and thus does NOT include retries. + * + * @param duration the Tunable[Duration] ([[org.joda.time.Duration]]) timeout to apply to requests through the filter. + * @see [[com.twitter.finagle.service.TimeoutFilter]] + * @return [[ThriftClientFilterChain]] + */ + def withRequestTimeout(duration: Tunable[Duration]): ThriftClientFilterChain[Req, Rep] = { + val exceptionFn = (duration: TwitterDuration) => + new IndividualRequestTimeoutException(duration * timeoutMultiplier) + + requestTimeoutFilter = new TimeoutFilter[Req, Rep]( + duration.map(d => d.toTwitterDuration), + exceptionFn, + DefaultTimer + ) + this + } + /** * Install a [[com.twitter.finagle.filter.RequestSemaphoreFilter]] using an [[com.twitter.concurrent.AsyncSemaphore]] * with an Optional `maxWaiters` as the limit on the number of waiters for permits. diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/GreeterFilteredThriftClientModule.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/GreeterFilteredThriftClientModule.scala index 8d297ceab2..2a3a6a0d13 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/GreeterFilteredThriftClientModule.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/filtered/GreeterFilteredThriftClientModule.scala @@ -5,7 +5,9 @@ import com.twitter.greeter.thriftscala.{Greeter, InvalidOperation} import com.twitter.inject.thrift.filters.ThriftClientFilterBuilder import com.twitter.inject.thrift.integration.filters.MethodLoggingTypeAgnosticFilter import com.twitter.inject.thrift.modules.FilteredThriftClientModule +import com.twitter.util.tunable.Tunable import com.twitter.util.{Future, Return, Throw} +import org.joda.time.Duration import scala.util.control.NonFatal object GreeterFilteredThriftClientModule @@ -20,6 +22,9 @@ object GreeterFilteredThriftClientModule filter: ThriftClientFilterBuilder ) = { + val timeoutTunable = Tunable.emptyMutable[Duration]("id") + timeoutTunable.set(new Duration(60000)) // 1.minute + serviceIface.copy( hi = filter .method(Hi) @@ -64,7 +69,7 @@ object GreeterFilteredThriftClientModule retries = 3 ) .withRequestLatency - .withRequestTimeout(1.minute) + .withRequestTimeout(timeoutTunable) .filtered[ByeThriftClientFilter] .andThen(serviceIface.bye) ) From c4dc773e401d5e8d4d2bd3d7259ee86d47020d32 Mon Sep 17 00:00:00 2001 From: Christopher Coco Date: Thu, 25 Jan 2018 01:40:07 +0000 Subject: [PATCH 07/12] inject-thrift-client: `ServicePerEndpointModule` -> `ThriftMethodBuilderClientModule` and update `ThriftClientModule` Summary: Problem The introduction of the `c.t.inject.thrift.modules.ServicePerEndpointModule` (now the `c.t.inject.thrift.modules.ThriftMethodBuilderClientModule`), gives users a way to configure a ThriftMux client with per-method semantics. While it is simple to not perform any configuration per-method to generate a simple client the construction of the module requires type information that is not strictly necessary. Additionally, the `ServicePerEndpointModule` only provides bindings to a `MyService.ServicePerEndpoint` and a `MyService.MethodPerEndpoint` which wraps the `MyService.ServicePerEndpoint`. If a user wants to create a simple ThriftMux client to a Thrift service with no per-method configuration or filtering, the `ThriftMethodBuilderClientModule` can be considered overkill. Solution Un-deprecate the `c.t.inject.thrift.modules.ThriftClientModule` and update it for parity in terms of creating and configuring the underlying ThriftMux client and swith to ThriftMux support *only*. Add documentation on it's intended use. THe `ThriftClientModule` is typed to a single interface param and exposes the configured client as a bound instance of that interface type. Supported types are dictated by the `c.t.finagle.thrift.ThriftRichClient`. We also update the `c.t.inject.thrift.modules.ServicePerEndpointModule` to the more consistently named: `c.t.inject.thrift.modules.ThriftMethodBuilderClientModule` Result User's have a choice of APIs when constructing a ThriftMux client in Finatra: they can use the more flexible per-method `ThriftMethodBuilderClientModule` or the simpler `ThriftClientModule` to just express ThriftMux client configuration with no per-method semantics and no filtering of the exposed client interface. TBR=true Differential Revision: https://phabricator.twitter.biz/D129891 --- CHANGELOG.md | 6 + doc/src/sphinx/user-guide/http/server.rst | 7 +- .../user-guide/testing/feature_tests.rst | 121 ++++++- .../user-guide/testing/integration_tests.rst | 21 +- doc/src/sphinx/user-guide/thrift/clients.rst | 339 ++++++++++++++---- doc/src/sphinx/user-guide/thrift/server.rst | 7 +- inject/inject-thrift-client/README.md | 90 ----- .../inject-thrift-client/src/main/scala/BUILD | 2 + .../filters/ThriftClientFilterBuilder.scala | 2 +- .../filters/ThriftClientFilterChain.scala | 2 +- .../modules/FilteredThriftClientModule.scala | 2 +- .../modules/ServicePerEndpointModule.scala | 231 ------------ .../thrift/modules/ThriftClientIdModule.scala | 6 + .../thrift/modules/ThriftClientModule.scala | 90 ++--- .../modules/ThriftClientModuleTrait.scala | 79 ++++ .../ThriftMethodBuilderClientModule.scala | 161 +++++++++ .../inject/thrift/modules/package.scala | 19 + ...ethodBuilderClientModuleFeatureTest.scala} | 10 +- ...rythingThriftClientModuleFeatureTest.scala | 181 +++++----- ...ethodBuilderClientModuleFeatureTest.scala} | 10 +- ...ethodBuilderClientModuleFeatureTest.scala} | 2 +- .../thrift/ThriftClientModuleNonMuxTest.scala | 27 -- .../thrift/integration/TestThriftServer.scala | 11 +- .../basic/EchoThriftClientModule1.scala | 18 + .../basic/EchoThriftClientModule2.scala | 10 + .../basic/EchoThriftClientModule3.scala | 10 + .../integration/basic/MyEchoService.scala | 31 ++ .../inheritance/ServiceBHttpServer.scala | 2 +- ...iceBThriftMethodBuilderClientModule.scala} | 6 +- ...qRepThriftMethodBuilderClientModule.scala} | 6 +- ...EchoThriftMethodBuilderClientModule.scala} | 6 +- ...eterThriftMethodBuilderClientModule.scala} | 6 +- 32 files changed, 909 insertions(+), 612 deletions(-) delete mode 100644 inject/inject-thrift-client/README.md delete mode 100644 inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ServicePerEndpointModule.scala create mode 100644 inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftClientModuleTrait.scala create mode 100644 inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftMethodBuilderClientModule.scala create mode 100644 inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/package.scala rename inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/{DoEverythingReqRepServicePerEndpointModuleFeatureTest.scala => DoEverythingReqRepThriftMethodBuilderClientModuleFeatureTest.scala} (91%) rename inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/{DoEverythingServicePerEndpointModuleFeatureTest.scala => DoEverythingThriftMethodBuilderClientModuleFeatureTest.scala} (96%) rename inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/{InheritanceServicePerEndpointModuleFeatureTest.scala => InheritanceThriftMethodBuilderClientModuleFeatureTest.scala} (98%) delete mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/ThriftClientModuleNonMuxTest.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/EchoThriftClientModule1.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/EchoThriftClientModule2.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/EchoThriftClientModule3.scala create mode 100644 inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/MyEchoService.scala rename inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/{ServiceBServicePerEndpointModule.scala => ServiceBThriftMethodBuilderClientModule.scala} (81%) rename inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/{GreeterReqRepServicePerEndpointModule.scala => GreeterReqRepThriftMethodBuilderClientModule.scala} (90%) rename inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/{EchoServicePerEndpointModule.scala => EchoThriftMethodBuilderClientModule.scala} (83%) rename inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/{GreeterServicePerEndpointModule.scala => GreeterThriftMethodBuilderClientModule.scala} (89%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 55cdf76af1..348283283c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,12 @@ All notable changes to this project will be documented in this file. Note that ` ### Changed +* inject-thrift-client: Un-deprecate `c.t.inject.thrift.modules.ThriftClientModule` + and update for parity with `ServicePerEndpointModule` in regards to ThriftMux + client configuration. Update documentation. Rename `ServicePerEndpointModule` to + the more descriptive and consistently named `ThriftMethodBuilderClientModule`. + ``PHAB_ID=D129891`` + ### Fixed ### Closed diff --git a/doc/src/sphinx/user-guide/http/server.rst b/doc/src/sphinx/user-guide/http/server.rst index bbe45a3b3f..9170e62668 100644 --- a/doc/src/sphinx/user-guide/http/server.rst +++ b/doc/src/sphinx/user-guide/http/server.rst @@ -152,4 +152,9 @@ For example: } -For more information on `Finagle `__ server configuration see the documentation `here `__; specifically the server documentation `here `__. \ No newline at end of file +For more information on `Finagle `__ server configuration see the documentation `here `__; specifically the server documentation `here `__. + +Testing +------- + +For information on testing an HTTP server see the HTTP Server `Feature Tests <../testing/feature_tests.html#http-server>`__ section. diff --git a/doc/src/sphinx/user-guide/testing/feature_tests.rst b/doc/src/sphinx/user-guide/testing/feature_tests.rst index 4df5fb2ca8..f410d967b8 100644 --- a/doc/src/sphinx/user-guide/testing/feature_tests.rst +++ b/doc/src/sphinx/user-guide/testing/feature_tests.rst @@ -3,14 +3,26 @@ Feature Tests ============= -If you are familiar with `Gherkin `__ or `Cucumber `__ or other similar testing languages and frameworks, then `feature testing `__ will feel somewhat familiar. In Finatra, a feature test always consists of an app or a server under test. See the `FeatureTest `__ trait. +.. note:: If you are calling an |c.t.util.Await|_ function on a |c.t.util.Future|_ return type in a + test, it is generally considered good practice to ensure that your |c.t.util.Await|_ call + includes a timeout duration, e.g., |c.t.util.Await#ready|_. -We highly recommend writing feature tests for your services as they provide a very good signal of whether you have correctly implemented the features of your service. If you haven't implemented the feature correctly, it almost doesn't matter that you have lots of unit tests. +If you are familiar with `Gherkin `__ or +`Cucumber `__ or other similar +testing languages and frameworks, then `feature testing `__ +will feel somewhat familiar. In Finatra, a feature test always consists of an app or a server under +test. See the `FeatureTest `__ +trait. + +We highly recommend writing feature tests for your services as they provide a very good signal of +whether you have correctly implemented the features of your service. If you haven't implemented the +feature correctly, it almost doesn't matter that you have lots of unit tests. HTTP Server ----------- -For example, to write a feature test for an HTTP server, extend the `c.t.inject.server.FeatureTest` trait. Then override the `server` with an instance of your |EmbeddedHttpServer|_. +For example, to write a feature test for an HTTP server, extend the `c.t.inject.server.FeatureTest` +trait. Then override the `server` definition with an instance of your |EmbeddedHttpServer|_. .. code:: scala @@ -32,39 +44,106 @@ For example, to write a feature test for an HTTP server, extend the `c.t.inject. Thrift Server ------------- -Similarly, to write a feature test for a Thrift server and create a `client <#thrift-tests>`__ to it, extend the `c.t.inject.server.FeatureTest` trait, override the `server` with an instance of your |EmbeddedThriftServer|_, and create a thrift client from the |EmbeddedThriftServer|_. +Similarly, to write a feature test for a Thrift server and create a `Finagle `__ +`client <#thrift-tests>`__ to it, extend the `c.t.inject.server.FeatureTest` trait, override the +`server` definition with an instance of your |EmbeddedThriftServer|_, and then create a Thrift client +from the |EmbeddedThriftServer|_. .. code:: scala + import com.example.thriftscala.ExampleThrift + import com.twitter.conversions.time._ import com.twitter.finatra.thrift.EmbeddedThriftServer import com.twitter.inject.server.FeatureTest + import com.twitter.util.Await class ExampleThriftServerFeatureTest extends FeatureTest { override val server = new EmbeddedThriftServer(new ExampleThriftServer) - lazy val client = server.thriftClient[ExampleThrift[Future]](clientId = "client123") + lazy val client: ExampleThrift[Future] = + server.thriftClient[ExampleThrift[Future]](clientId = "client123") test("ExampleThriftServer#return data accordingly") { - Await.result(client.doExample("input")) should equal("output") - } + Await.result(client.doExample("input"), 2.seconds) should equal("output") } } +Client Interface Types +~~~~~~~~~~~~~~~~~~~~~~ + +As mentioned in the Scrooge `Finagle Integration `__ +documentation, users have three API choices for building an interface to a Finagle Thrift client — +``ServicePerEndpoint``, ``ReqRepServicePerEndpoint``, and ``MethodPerEndpoint``. This is true even +when creating a test Thrift client to a Thrift server. + +In the example above, we create a Thrift client in the form of the higher-kinded type interface, +e.g., `MyService[+MM[_]]`. We could choose to create a `ExampleThrift.MethodPerEndpoint` +interface instead by changing the type parameter given to the |c.t.finatra.thrift.ThriftClient#thriftClient[T]|_ +method: + +.. code:: scala + + lazy val client: ExampleThrift.MethodPerEndpoint = + server.thriftClient[ExampleThrift.MethodPerEndpoint](clientId = "client123") + +Users can also choose to create a `service-per-endpoint` Thrift client interface by calling the +|c.t.finatra.thrift.ThriftClient#servicePerEndpoint[T]|_ with either the ``ServicePerEndpoint`` or +``ReqReServicePerEndpoint`` type. E.g., + +.. code:: scala + + lazy val client: ExampleThrift.ServicePerEndpoint = + server.servicePerEndpoint[ExampleThrift.ServicePerEndpoint](clientId = "client123") + +or + +.. code:: scala + + lazy val client: ExampleThrift.ReqRepServicePerEndpoint = + server.servicePerEndpoint[ExampleThrift.ReqRepServicePerEndpoint](clientId = "client123") + +Lastly, the Thrift client can also be expressed as a ``MethodPerEndpoint`` wrapping a +`service-per-endpoint` type by using |c.t.finatra.thrift.ThriftClient#methodPerEndpoint[T, U]|_. +This would allow for applying a set of filters on the Thrift client interface before interacting +with the Thrift client as a ``MethodPerEndpoint`` interface. + +For example: + +.. code:: scala + + lazy val servicePerEndpoint: ExampleThrift.ServicePerEndpoint = + server + .servicePerEndpoint[ExampleThrift.ServicePerEndpoint](clientId = "client123") + .filtered(???) + + lazy val client: ExampleThrift.MethodPerEndpoint = + server.methodPerEndpoint[ + ExampleThrift.ServicePerEndpoint, + ExampleThrift.MethodPerEndpoint](servicePerEndpoint) + +See the `Communicate with a Thrift Service <../thrift/clients.html>`__ section for more information +on Thrift clients. + Combined HTTP & Thrift Server ----------------------------- -If you are extending both `c.t.finatra.http.HttpServer` **and** `c.t.finatra.thrift.ThriftServer` then you can feature test by constructing an `EmbeddedHttpServer with ThriftClient`, e.g., +If you are extending both `c.t.finatra.http.HttpServer` **and** `c.t.finatra.thrift.ThriftServer` +then you can feature test by constructing an `EmbeddedHttpServer with ThriftClient`, e.g., .. code:: scala + import com.example.thriftscala.ExampleThrift + import com.twitter.conversions.time._ import com.twitter.finatra.http.EmbeddedHttpServer + import com.twitter.finatra.thrift.ThriftClient import com.twitter.inject.server.FeatureTest class ExampleCombinedServerFeatureTest extends FeatureTest { override val server = new EmbeddedHttpServer(new ExampleCombinedServer) with ThriftClient - lazy val client = server.thriftClient[ExampleThrift[Future]](clientId = "client123") + lazy val client: ExampleThrift[Future] = + server.thriftClient[ExampleThrift[Future]](clientId = "client123") "ExampleCombinedServer#perform feature") { server.httpGet( @@ -74,7 +153,7 @@ If you are extending both `c.t.finatra.http.HttpServer` **and** `c.t.finatra.thr } "ExampleCombinedServer#return data accordingly") { - Await.result(client.doExample("input")) should equal("output") + Await.result(client.doExample("input"), 2.seconds) should equal("output") } } } @@ -82,7 +161,10 @@ If you are extending both `c.t.finatra.http.HttpServer` **and** `c.t.finatra.thr .. caution:: - The `server` is specified as a `def` in the |c.t.inject.server.FeatureTestMixin|_ trait. If you only want to start **one instance of your server per test file** make sure to override this `def` with a `val`. + The `server` is specified as a `def` in the |c.t.inject.server.FeatureTestMixin|_ trait. + + If you only want to start **one instance** of your server per test file make sure to override this + `def` with a `val`. For more advanced examples see: @@ -118,3 +200,20 @@ More Information .. |EmbeddedThriftServer| replace:: `EmbeddedThriftServer` .. _EmbeddedThriftServer: https://github.com/twitter/finatra/blob/develop/thrift/src/test/scala/com/twitter/finatra/thrift/EmbeddedThriftServer.scala +.. |c.t.finatra.thrift.ThriftClient#thriftClient[T]| replace:: `c.t.finatra.thrift.ThriftClient#thriftClient[T]` +.. _c.t.finatra.thrift.ThriftClient#thriftClient[T]: https://github.com/twitter/finatra/blob/72664be4439da4425dfe63fa325f4c1ebbc5bf4b/thrift/src/test/scala/com/twitter/finatra/thrift/ThriftClient.scala#L77 + +.. |c.t.finatra.thrift.ThriftClient#servicePerEndpoint[T]| replace:: `c.t.finatra.thrift.ThriftClient#servicePerEndpoint[T]` +.. _c.t.finatra.thrift.ThriftClient#servicePerEndpoint[T]: https://github.com/twitter/finatra/blob/72664be4439da4425dfe63fa325f4c1ebbc5bf4b/thrift/src/test/scala/com/twitter/finatra/thrift/ThriftClient.scala#L103 + +.. |c.t.finatra.thrift.ThriftClient#methodPerEndpoint[T, U]| replace:: `c.t.finatra.thrift.ThriftClient#methodPerEndpoint[T, U]` +.. _c.t.finatra.thrift.ThriftClient#methodPerEndpoint[T, U]: https://github.com/twitter/finatra/blob/72664be4439da4425dfe63fa325f4c1ebbc5bf4b/thrift/src/test/scala/com/twitter/finatra/thrift/ThriftClient.scala#L134 + +.. |c.t.util.Await| replace:: `c.t.util.Await` +.. _c.t.util.Await: https://github.com/twitter/util/blob/54f314d1f4b37d302f685e99b1ac416e48532a04/util-core/src/main/scala/com/twitter/util/Awaitable.scala#L77 + +.. |c.t.util.Future| replace:: `c.t.util.Future` +.. _c.t.util.Future: https://github.com/twitter/util/blob/develop/util-core/src/main/scala/com/twitter/util/Future.scala + +.. |c.t.util.Await#ready| replace:: `c.t.util.Await#ready` +.. _c.t.util.Await#ready: https://github.com/twitter/util/blob/54f314d1f4b37d302f685e99b1ac416e48532a04/util-core/src/main/scala/com/twitter/util/Awaitable.scala#L127 diff --git a/doc/src/sphinx/user-guide/testing/integration_tests.rst b/doc/src/sphinx/user-guide/testing/integration_tests.rst index 635c51e775..1a71099e74 100644 --- a/doc/src/sphinx/user-guide/testing/integration_tests.rst +++ b/doc/src/sphinx/user-guide/testing/integration_tests.rst @@ -3,7 +3,11 @@ Integration Tests ================= -Whereas `Feature Tests <#feature_tests>`__ start a server or app under test (thereby loading its +.. note:: If you are calling an |c.t.util.Await|_ function on a |c.t.util.Future|_ return type in a + test, it is generally considered good practice to ensure that your |c.t.util.Await|_ call + includes a timeout duration, e.g., |c.t.util.Await#ready|_. + +Whereas `Feature Tests `__ start a server or app under test (thereby loading its entire object graph), integration tests generally only test across a few interfaces in the system. In Finatra, we provide the `c.t.inject.app.TestInjector `__ which allows you to pass it a set of modules and flags to construct a minimal object graph. @@ -53,9 +57,8 @@ As shown above, thrift servers can be tested through a |c.t.finatra.thrift.Thrif Finatra test framework provides an easy way get access to a real `Finagle client `__ for making calls to your running server in a test. -In the case here, creating a |c.t.finatra.thrift.ThriftClient|_ requires the thrift service type -`T`. This type is expected to be the trait subclass of `c.t.scrooge.ThriftService` in the form of -`YourService[+MM[_]]`. +See the `Feature Tests - Thrift Server `__ section for more +information on creating a Finagle Thrift client. Additionally, your test can also extend the |c.t.finatra.thrift.ThriftTest|_ trait which provides a utility specifically for constructing a |resolverMap|_ flag value for setting on your server under @@ -88,3 +91,13 @@ More Information .. |resolverMap| replace:: `resolverMap` .. _resolverMap: https://github.com/twitter/twitter-server/blob/15e35a3a3070c50168ff55fd83a2dff28b09795c/server/src/main/scala/com/twitter/server/FlagResolver.scala#L9> + +.. |c.t.util.Await| replace:: `c.t.util.Await` +.. _c.t.util.Await: https://github.com/twitter/util/blob/54f314d1f4b37d302f685e99b1ac416e48532a04/util-core/src/main/scala/com/twitter/util/Awaitable.scala#L77 + +.. |c.t.util.Future| replace:: `c.t.util.Future` +.. _c.t.util.Future: https://github.com/twitter/util/blob/develop/util-core/src/main/scala/com/twitter/util/Future.scala + +.. |c.t.util.Await#ready| replace:: `c.t.util.Await#ready` +.. _c.t.util.Await#ready: https://github.com/twitter/util/blob/54f314d1f4b37d302f685e99b1ac416e48532a04/util-core/src/main/scala/com/twitter/util/Awaitable.scala#L127 + diff --git a/doc/src/sphinx/user-guide/thrift/clients.rst b/doc/src/sphinx/user-guide/thrift/clients.rst index e508f46aaa..a255956694 100644 --- a/doc/src/sphinx/user-guide/thrift/clients.rst +++ b/doc/src/sphinx/user-guide/thrift/clients.rst @@ -10,14 +10,29 @@ As mentioned in the Scrooge `Finagle Integration `__ +of `ThriftMethod.Args` to `ThriftMethod.SuccessType`: + +.. code:: scala + + Service[method.Args, method.SuccessType] + +By their nature of being |Services|_, the methods are composable with Finagle's |Filters|_. The ``ReqRepServicePerEndpoint`` interface is also a collection of Finagle |Services|_, however methods are |Services|_ from a |c.t.scrooge.Request|_ to a |c.t.scrooge.Response|_, e.g., -`Service[Request[method.Args], Response[method.SuccessType]]`. These envelope types allow for the -passing of header information between clients and servers when using the `com.twitter.finagle.ThriftMux` + +.. code:: scala + + import com.twitter.scrooge.{Request, Response} + + Service[Request[method.Args], Response[method.SuccessType]] + +These envelope types allow for the passing of header information between clients and servers when +using the Finagle `ThriftMux `__ protocol. Getting Started @@ -31,42 +46,52 @@ E.g., with sbt: "com.twitter" %% "inject-thrift-client" % "\ |release|\ ", -|ServicePerEndpointModule|_ ----------------------------------------------- +|ThriftMethodBuilderClientModule|_ +---------------------------------- -.. note:: Currently the |c.t.inject.thrift.modules.ServicePerEndpointModule|_ only supports the -ThriftMux protocol. +.. note:: The |c.t.inject.thrift.modules.ThriftMethodBuilderClientModule|_ only supports the + `ThriftMux `__ + protocol. -The |c.t.inject.thrift.modules.ServicePerEndpointModule|_ allows for configuration of a Finagle -Thrift client using the Finagle ThriftMux `MethodBuilder `__ -integration. +The |c.t.inject.thrift.modules.ThriftMethodBuilderClientModule|_ allows for configuration of a Finagle +Thrift client using the Finagle `ThriftMux `__ +`MethodBuilder `__ integration. Users have the option to configure a `service-per-endpoint`, i.e., ``ServicePerEndpoint`` or -``ReqRepServicePerEndpoint`` interface of a Thrift client and the |c.t.inject.thrift.modules.ServicePerEndpointModule|_ +``ReqRepServicePerEndpoint`` interface of the Thrift client and the |c.t.inject.thrift.modules.ThriftMethodBuilderClientModule|_ provides bindings for both the chosen `service-per-endpoint` interface and the ``MethodPerEndpoint`` -interface. In this case, the ``MethodPerEndpoint`` interface functions as a thin wrapper the +interface. In this case, the ``MethodPerEndpoint`` interface functions as a thin wrapper over the configured `service-per-endpoint`. The choice to interact with Finagle |Services|_ when calling the configured Thrift client or to use the Thrift method interface is up to you. You will have access to both in the object graph when -implementing a |c.t.inject.thrift.modules.ServicePerEndpointModule|_. +implementing a |ThriftMethodBuilderClientModule|_. To create a new client, first create a new `TwitterModule <../getting-started/modules.html>`_ which -extends |c.t.inject.thrift.modules.ServicePerEndpointModule|_: +extends |c.t.inject.thrift.modules.ThriftMethodBuilderClientModule|_: .. code:: scala - import com.twitter.inject.thrift.modules.ServicePerEndpointModule + import com.twitter.finagle.Budget + import com.twitter.inject.thrift.modules.ThriftMethodBuilderClientModule import com.twitter.myservice.thriftscala.MyService + import com.twitter.util.{Duration, Monitor} object MyServiceModule - extends ServicePerEndpointModule[MyService.ServicePerEndpoint, MyService.MethodPerEndpoint] { + extends ThriftMethodBuilderClientModule[ + MyService.ServicePerEndpoint, + MyService.MethodPerEndpoint] { + + override val dest: String = "flag!myservice-thrift-service" + override val label: String = "myservice-thrift-client" - override val dest = "flag!myservice-thrift-service" - override val label = "myservice-thrift-client" + override val sessionAcquisitionTimeout: Duration = ??? + override val requestTimeout: Duration = ??? + override val retryBudget: Budget = ??? + override val monitor: Monitor = ??? } -The |ServicePerEndpointModule|_ implementation must be typed to a Scrooge-generated `service-per-endpoint` +The |ThriftMethodBuilderClientModule|_ implementation must be typed to a Scrooge-generated `service-per-endpoint` and the ``MethodPerEndpoint``. Users can choose either the `MyService.ServicePerEndpoint` interface (as in the above example) or the `MyService.ReqRepServicePerEndpoint` interface depending on their requirements: @@ -74,38 +99,53 @@ requirements: .. code:: scala object MyServiceModule - extends ServicePerEndpointModule[MyService.ReqRepServicePerEndpoint, MyService.MethodPerEndpoint] { + extends ThriftMethodBuilderClientModule[ + MyService.ReqRepServicePerEndpoint, + MyService.MethodPerEndpoint] { ... -At a minimum, to use the |c.t.inject.thrift.modules.ServicePerEndpointModule|_, a ThriftMux -`client label `__ and String -`dest `__ MUST also be specified. - -See `here `__ for more information on Finagle -clients. +At a minimum, to use the |c.t.inject.thrift.modules.ThriftMethodBuilderClientModule|_, a +`ThriftMux `__ +`client label `__ and a String +`dest `__ **must** be specified. Configuration ~~~~~~~~~~~~~ -The |ServicePerEndpointModule|_ intends to allow users to configure ThriftMux client +The |ThriftMethodBuilderClientModule|_ intends to allow users to configure +`ThriftMux `__ client semantics and apply filters per-method via the |c.t.inject.thrift.ThriftMethodBuilder|_ which is a -thin wrapper over the Finagle ThriftMux `MethodBuilder `__. +thin wrapper over the Finagle `ThriftMux `__ +`MethodBuilder `__. + +Advanced `ThriftMux `__ client +configuration can be done by overriding the `ThriftMethodBuilderClientModule#configureThriftMuxClient` +method which allows for ad-hoc `ThriftMux `__ +client configuration. + +See `Finagle Client Modules `__ +for more information on client configuration parameters and their meanings. + +Per-Method Configuration +~~~~~~~~~~~~~~~~~~~~~~~~ To configure per-method semantics, override and provide an implementation for the -`ServicePerEndpointModule#configureServicePerEndpointModule` method. E.g., +`ThriftMethodBuilderClientModule#configureServicePerEndpoint` method. E.g., .. code:: scala import com.twitter.inject.Injector import com.twitter.inject.thrift.ThriftMethodBuilderFactory - import com.twitter.inject.thrift.modules.ServicePerEndpointModule + import com.twitter.inject.thrift.modules.ThriftMethodBuilderClientModule import com.twitter.myservice.thriftscala.MyService object MyServiceModule - extends ServicePerEndpointModule[MyService.ServicePerEndpoint, MyService.MethodPerEndpoint] { + extends ThriftMethodBuilderClientModule[ + MyService.ServicePerEndpoint, + MyService.MethodPerEndpoint] { - override val dest = "flag!myservice-thrift-service" - override val label = "myservice-thrift-client" + override val dest: String = "flag!myservice-thrift-service" + override val label: String = "myservice-thrift-client" override def configureServicePerEndpoint( injector: Injector, @@ -131,34 +171,37 @@ To configure per-method semantics, override and provide an implementation for th In this example we are configuring the given `servicePerEndpoint` by re-implementing the `Foo` and `Bar` functions using a "builder"-like API. Each `Scrooge `__-generated -client-side `ServicePerEndpoint` provides a `withXXXX` function over every defined Thrift method that +client-side ``ServicePerEndpoint`` provides a `withXXXX` function over every defined Thrift method that allows users to replace the current implementation of the method with a new implementation. The replacement must still be a correctly-typed Finagle `Service`. In the above example we replace the methods with implementations built up from a combination of `MethodBuilder `__ functionality and arbitrary filters ending with a call to `ThriftMethodBuilder#service` which materializes the -resultant `Service`. +resultant `Service[-Req, +Rep]`. + +Global Filters +^^^^^^^^^^^^^^ Note that `TypeAgnostic `__ -Finagle |Filters|_ can also be applied "globally" across the all methods of a `ServicePerEndpoint` -interface by calling ``ServicePerEndpoint#filtered``. +Finagle |Filters|_ can also be applied "globally" across the all methods of a ``ServicePerEndpoint`` +interface by calling `ServicePerEndpoint#filtered`. For example, to apply a set of `TypeAgnostic `__ -Finagle Filters to a `ServicePerEndpoint`: +Finagle Filters to a ``ServicePerEndpoint``: .. code:: scala import com.twitter.inject.Injector import com.twitter.inject.thrift.ThriftMethodBuilderFactory - import com.twitter.inject.thrift.modules.ServicePerEndpointModule + import com.twitter.inject.thrift.modules.ThriftMethodBuilderClientModule import com.twitter.myservice.thriftscala.MyService object MyServiceModule - extends ServicePerEndpointModule[MyService.ServicePerEndpoint, MyService.MethodPerEndpoint] { + extends ThriftMethodBuilderClientModule[MyService.ServicePerEndpoint, MyService.MethodPerEndpoint] { - override val dest = "flag!myservice-thrift-service" - override val label = "myservice-thrift-client" + override val dest: String = "flag!myservice-thrift-service" + override val label: String = "myservice-thrift-client" override def configureServicePerEndpoint( injector: Injector, @@ -173,36 +216,33 @@ Finagle Filters to a `ServicePerEndpoint`: This can be combined with the per-method configuration as well. -.. admonition:: More Information on `Modules <../getting-started/modules.html>`__: - - Module `best practices <../getting-started/modules.html#best-practices>`__ - and `depending on other modules <../getting-started/modules.html#modules-depending-on-other-modules>`__. - -Bindings -~~~~~~~~ +ThriftMethodBuilderClientModule Bindings +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ When included in a server's `module <../getting-started/modules.html>`__ list, an implementation -of the |ServicePerEndpointModule|_ will provide bindings to both +of the |ThriftMethodBuilderClientModule|_ will provide bindings to both `MyService.ServicePerEndpoint `__ (or `MyService.ReqRepServicePerEndpoint `__) **and** `MyService.MethodPerEndpoint `__. -For example, given the following |ServicePerEndpointModule|_ implementation: +For example, given the following |ThriftMethodBuilderClientModule|_ implementation: .. code:: scala - import com.twitter.inject.thrift.modules.ServicePerEndpointModule + import com.twitter.inject.thrift.modules.ThriftMethodBuilderClientModule import com.twitter.myservice.thriftscala.MyService object MyServiceModule - extends ServicePerEndpointModule[MyService.ServicePerEndpoint, MyService.MethodPerEndpoint] { + extends ThriftMethodBuilderClientModule[ + MyService.ServicePerEndpoint, + MyService.MethodPerEndpoint] { - override val dest = "flag!myservice-thrift-service" - override val label = "myservice-thrift-client" + override val dest: String = "flag!myservice-thrift-service" + override val label: String = "myservice-thrift-client" } -This means that both `MyService.ServicePerEndpoint` and `MyService.MethodPerEndpoint` will be -injectable. Which to use is dependent on your use-case. +This means that both the `MyService.ServicePerEndpoint` and `MyService.MethodPerEndpoint` types will +be injectable. Which to use is dependent on your use-case. .. code:: scala @@ -220,21 +260,154 @@ injectable. Which to use is dependent on your use-case. } } +|ThriftClientModule|_ +--------------------- + +.. note:: The |c.t.inject.thrift.modules.ThriftClientModule|_ only supports the + `ThriftMux `__ + protocol. + +The |c.t.inject.thrift.modules.ThriftClientModule|_ allows for simpler configuration of a Finagle +Thrift client than the |c.t.inject.thrift.modules.ThriftMethodBuilderClientModule|_. + +Users have the option of configuring either a ``MethodPerEndpoint`` or the higher-kinded, e.g., +``MyService[+MM[_]]``, interface of the Thrift client and the |c.t.inject.thrift.modules.ThriftClientModule|_ +provides a binding to the chosen interface. + +To create a new client, first create a new `TwitterModule <../getting-started/modules.html>`_ which +extends |c.t.inject.thrift.modules.ThriftClientModule|_: + +.. code:: scala + + import com.twitter.inject.thrift.modules.ThriftClientModule + import com.twitter.myservice.thriftscala.MyService + + object MyServiceModule + extends ThriftClientModule[MyService.MethodPerEndpoint] { + + override val dest: String = "flag!myservice-thrift-service" + override val label: String = "myservice-thrift-client" + } + +The |ThriftClientModule|_ implementation must be typed to either the Scrooge-generated +``MethodPerEndpoint`` or the ``MyService[+MM[_]]`` Thrift service interface. These interfaces are +semantically equivalent, however there are differences when it comes to some testing features which +have trouble dealing with higher-kinded types (like mocking). + +Users can choose either interface depending on their requirements. E.g., to use the ``MyService[+MM[_]]`` +interface for `MyService`: + +.. code:: scala + + object MyServiceModule + extends ThriftClientModule[MyService[Future]] { + ... + +At a minimum, to use the |c.t.inject.thrift.modules.ThriftClientModule|_, a +`ThriftMux `__ +`client label `__ and a String +`dest `__ **must** be specified. + +Configuration +~~~~~~~~~~~~~ + +The |ThriftClientModule|_ intends to allow users to easily configure common parameters of a +`ThriftMux `__ client. + +.. code:: scala + + import com.twitter.finagle.Budget + import com.twitter.inject.Injector + import com.twitter.inject.thrift.ThriftMethodBuilderFactory + import com.twitter.inject.thrift.modules.ThriftClientModule + import com.twitter.myservice.thriftscala.MyService + import com.twitter.util.{Duration, Monitor} + + object MyServiceModule + extends ThriftClientModule[MyService.MethodPerEndpoint] { + + override val dest: String = "flag!myservice-thrift-service" + override val label: String = "myservice-thrift-client" + + override val sessionAcquisitionTimeout: Duration = ??? + + override val requestTimeout: Duration = ??? + + override val retryBudget: Budget = ??? + + override val monitor: Monitor = ??? + +Advanced `ThriftMux `__ +configuration can be done by overriding the `ThriftClientModule#configureThriftMuxClient` +method which allows for ad-hoc `ThriftMux `__ +client configuration. + +See `Finagle Client Modules `__ +for more information on client configuration parameters and their meanings. + +ThriftClientModule Bindings +~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +When included in a server's `module <../getting-started/modules.html>`__ list, an implementation +of the |ThriftClientModule|_ will provide a binding of the specified type param to the object graph. +Either `MyService.MethodPerEndpoint `__ or +`MyService[Future]`. + +For example, given the following |ThriftClientModule|_ implementation: + +.. code:: scala + + import com.twitter.inject.thrift.modules.ThriftClientModule + import com.twitter.myservice.thriftscala.MyService + + object MyServiceModule + extends ThriftClientModule[MyService.MethodPerEndpoint] { + + override val dest: String = "flag!myservice-thrift-service" + override val label: String = "myservice-thrift-client" + } + +This means that the `MyService.MethodPerEndpoint` type will be injectable. + +.. code:: scala + + import com.twitter.finagle.http.Request + import com.twitter.finatra.http.Controller + import com.twitter.myservice.thriftscala.MyService + import javax.inject.{Inject, Singleton} + + @Singleton + class MyDataController @Inject()( + myService: MyService.MethodPerEndpoint + ) extends Controller { + get("/") { request: Request => + myService.foo(request.params("data")) + } + } + More Information ---------------- -For more information, see the `Finagle Integration `__ -section of the `Scrooge `__ documentation. +.. admonition:: More Information on `Modules <../getting-started/modules.html>`__: + + Module `best practices <../getting-started/modules.html#best-practices>`__ + and `depending on other modules <../getting-started/modules.html#modules-depending-on-other-modules>`__. + +For more information on `Scrooge `__-generated client +interfaces see the `Finagle Integration `__ section +of the `Scrooge `__ documentation. More detailed examples are available in the integration tests: -- |DoEverythingServicePerEndpointModuleFeatureTest|_ and -- |DoEverythingReqRepServicePerEndpointModuleFeatureTest|_ +- |DoEverythingThriftClientModuleFeatureTest|_ +- |DoEverythingThriftMethodBuilderClientModuleFeatureTest|_ +- |DoEverythingReqRepThriftMethodBuilderClientModuleFeatureTest|_ -which test over two different implementations of a |ServicePerEndpointModule|_: +which test over multiple implementations of a |ThriftClientModule|_ and |ThriftMethodBuilderClientModule|_: -- |GreeterServicePerEndpointModule|_ and -- |GreeterReqRepServicePerEndpointModule|_ respectively. +- |EchoThriftClientModules|_ +- |GreeterThriftMethodBuilderClientModule|_ +- |GreeterReqRepThriftMethodBuilderClientModule|_. .. |Services| replace:: `Services` .. _Services: https://twitter.github.io/finagle/guide/ServicesAndFilters.html#services @@ -251,20 +424,32 @@ which test over two different implementations of a |ServicePerEndpointModule|_: .. |c.t.inject.thrift.ThriftMethodBuilder| replace:: ``c.t.inject.thrift.ThriftMethodBuilder`` .. _c.t.inject.thrift.ThriftMethodBuilder: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/ThriftMethodBuilder.scala -.. |c.t.inject.thrift.modules.ServicePerEndpointModule| replace:: `c.t.inject.thrift.modules.ServicePerEndpointModule` -.. _c.t.inject.thrift.modules.ServicePerEndpointModule: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ServicePerEndpointModule.scala +.. |c.t.inject.thrift.modules.ThriftMethodBuilderClientModule| replace:: `c.t.inject.thrift.modules.ThriftMethodBuilderClientModule` +.. _c.t.inject.thrift.modules.ThriftMethodBuilderClientModule: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftMethodBuilderClientModule.scala + +.. |ThriftMethodBuilderClientModule| replace:: `ThriftMethodBuilderClientModule` +.. _ThriftMethodBuilderClientModule: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftMethodBuilderClientModule.scala + +.. |ThriftClientModule| replace:: `ThriftClientModule` +.. _ThriftClientModule: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftClientModule.scala + +.. |c.t.inject.thrift.modules.ThriftClientModule| replace:: `c.t.inject.thrift.modules.ThriftClientModule` +.. _c.t.inject.thrift.modules.ThriftClientModule: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftClientModule.scala + +.. |DoEverythingThriftClientModuleFeatureTest| replace:: `DoEverythingThriftClientModuleFeatureTest` +.. _DoEverythingThriftClientModuleFeatureTest: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingThriftClientModuleFeatureTest.scala -.. |ServicePerEndpointModule| replace:: `ServicePerEndpointModule` -.. _ServicePerEndpointModule: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ServicePerEndpointModule.scala +.. |EchoThriftClientModules| replace:: `EchoThriftClientModules` +.. _EchoThriftClientModules: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic -.. |DoEverythingServicePerEndpointModuleFeatureTest| replace:: `DoEverythingServicePerEndpointModuleFeatureTest` -.. _DoEverythingServicePerEndpointModuleFeatureTest: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingServicePerEndpointModuleFeatureTest.scala +.. |DoEverythingThriftMethodBuilderClientModuleFeatureTest| replace:: `DoEverythingThriftMethodBuilderClientModuleFeatureTest` +.. _DoEverythingThriftMethodBuilderClientModuleFeatureTest: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingThriftMethodBuilderClientModuleFeatureTest.scala -.. |GreeterServicePerEndpointModule| replace:: `GreeterServicePerEndpointModule` -.. _GreeterServicePerEndpointModule: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterServicePerEndpointModule.scala +.. |GreeterThriftMethodBuilderClientModule| replace:: `GreeterThriftMethodBuilderClientModule` +.. _GreeterThriftMethodBuilderClientModule: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterThriftMethodBuilderClientModule.scala -.. |DoEverythingReqRepServicePerEndpointModuleFeatureTest| replace:: `DoEverythingReqRepServicePerEndpointModuleFeatureTest` -.. _DoEverythingReqRepServicePerEndpointModuleFeatureTest: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingReqRepServicePerEndpointModuleFeatureTest.scala +.. |DoEverythingReqRepThriftMethodBuilderClientModuleFeatureTest| replace:: `DoEverythingReqRepThriftMethodBuilderClientModuleFeatureTest` +.. _DoEverythingReqRepThriftMethodBuilderClientModuleFeatureTest: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingReqRepThriftMethodBuilderClientModuleFeatureTest.scala -.. |GreeterReqRepServicePerEndpointModule| replace:: `GreeterReqRepServicePerEndpointModule` -.. _GreeterReqRepServicePerEndpointModule: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepServicePerEndpointModule.scala +.. |GreeterReqRepThriftMethodBuilderClientModule| replace:: `GreeterReqRepThriftMethodBuilderClientModule` +.. _GreeterReqRepThriftMethodBuilderClientModule: https://github.com/twitter/finatra/blob/develop/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepThriftMethodBuilderClientModule.scala diff --git a/doc/src/sphinx/user-guide/thrift/server.rst b/doc/src/sphinx/user-guide/thrift/server.rst index b8be01ad10..eb2dadb021 100644 --- a/doc/src/sphinx/user-guide/thrift/server.rst +++ b/doc/src/sphinx/user-guide/thrift/server.rst @@ -120,4 +120,9 @@ For example: } -For more information on `Finagle `__ server configuration see the documentation `here `__; specifically the server documentation `here `__. \ No newline at end of file +For more information on `Finagle `__ server configuration see the documentation `here `__; specifically the server documentation `here `__. + +Testing +------- + +For information on testing a Thrift server see the Thrift Server `Feature Tests <../testing/feature_tests.html#thrift-server>`__ section. diff --git a/inject/inject-thrift-client/README.md b/inject/inject-thrift-client/README.md deleted file mode 100644 index 6bf242c18d..0000000000 --- a/inject/inject-thrift-client/README.md +++ /dev/null @@ -1,90 +0,0 @@ -# Summary -inject-thrift-client is a library for configuring injectable thrift clients. The examples below demonstrate creating a thrift-client for the following thrift defined service: -```thrift -exception InvalidOperation { - 1: i32 what, - 2: string why -} - -exception ByeOperation { - 1: i32 code -} - -struct ByeResponse { - 1: double code; - 2: string msg; -} - -service Greeter { - - /** - * Say hi - */ - string hi( - 1: string name - ) throws (1:InvalidOperation invalidOperation) - - /** - * Say bye - */ - ByeResponse bye( - 1: string name - 2: i32 age - ) throws (1:ByeOperation byeOperation) -} -``` - -## FilteredThriftClientModule Usage -FilteredThriftClientModule integrates with Scrooge 4 ["services-per-endpoint"](https://finagle.github.io/blog/2015/09/10/services-per-endpoint-in-scrooge/), supporting per-method filter chains which can retry on requests, successful responses, failed futures, and thrift-idl defined exceptions. - -```scala -object GreeterThriftClientModule - extends FilteredThriftClientModule[Greeter[Future], Greeter.ServiceIface] { - - override val label = "greeter-thrift-client" - override val dest = "flag!greeter-thrift-service" - override val sessionAcquisitionTimeout = 1.minute.toDuration - - override def filterServiceIface( - serviceIface: Greeter.ServiceIface, - filter: ThriftClientFilterBuilder) = { - - serviceIface.copy( - hi = filter.method(Hi) - .withTimeout(2.minutes) - .withConstantRetry( - shouldRetryResponse = { - case Return(Hi.Result(_, Some(e: InvalidOperation))) => true - case Return(Hi.Result(Some(success), _)) => success == "ERROR" - case Throw(NonFatal(_)) => true - }, - start = 50.millis, - retries = 3) - .withRequestTimeout(1.minute) - .filtered[HiLoggingThriftClientFilter] - .andThen(serviceIface.hi), - bye = filter.method(Bye) - .withExponentialRetry( - shouldRetryResponse = PossiblyRetryableExceptions, - start = 50.millis, - multiplier = 2, - retries = 3) - .withRequestTimeout(1.minute) - .andThen(serviceIface.bye)) - } -``` - -Then add GreeterThriftClientModule to your list of server modules, and inject the "Greeter" thrift-client as such: -```scala -class MyClass @Inject()( - greeter: Greeter[Future]) -``` - -## ThriftClientModule Usage (deprecated) -If you don't need client retries or other client filters, ThriftClientModule can be used to simply create an injectable thrift client as such: -```scala -object GreeterThriftClientModule extends ThriftClientModule[Greeter[Future]] { - override val label = "greeter-thrift-client" - override val dest = "flag!greeter-thrift-service" -} -``` diff --git a/inject/inject-thrift-client/src/main/scala/BUILD b/inject/inject-thrift-client/src/main/scala/BUILD index 0d27d91790..a50b7db661 100644 --- a/inject/inject-thrift-client/src/main/scala/BUILD +++ b/inject/inject-thrift-client/src/main/scala/BUILD @@ -37,6 +37,8 @@ scala_library( '3rdparty/jvm/com/github/nscala_time:nscala_time', '3rdparty/jvm/com/google/inject/extensions:guice-assistedinject', 'finagle/finagle-core/src/main/scala:scala', + 'finagle/finagle-thrift/src/main/scala', + 'finagle/finagle-thriftmux/src/main/scala', 'finatra/inject/inject-app/src/main/scala:scala', 'scrooge/scrooge-core/src/main/scala:scala', ], diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterBuilder.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterBuilder.scala index 92da0037d1..9f90486b29 100644 --- a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterBuilder.scala +++ b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterBuilder.scala @@ -6,7 +6,7 @@ import com.twitter.inject.Injector import com.twitter.inject.thrift.AndThenService import com.twitter.scrooge.ThriftMethod -@deprecated("Use ServicePerEndpointModule and ThriftMethodBuilder", "2018-01-12") +@deprecated("Use ThriftMethodBuilderClientModule and ThriftMethodBuilder", "2018-01-12") class ThriftClientFilterBuilder( timeoutMultiplier: Int, retryMultiplier: Int, diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterChain.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterChain.scala index a1885dd481..cf27bf5b3d 100644 --- a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterChain.scala +++ b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/filters/ThriftClientFilterChain.scala @@ -71,7 +71,7 @@ import org.joda.time.Duration * @see [[com.twitter.inject.thrift.filters.ThriftClientFilterBuilder]] * @see [[com.twitter.finagle.thrift.ThriftServiceIface]] */ -@deprecated("Use ServicePerEndpointModule and ThriftMethodBuilder", "2018-01-12") +@deprecated("Use ThriftMethodBuilderClientModule and ThriftMethodBuilder", "2018-01-12") class ThriftClientFilterChain[Req <: ThriftStruct, Rep]( injector: Injector, statsReceiver: StatsReceiver, diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/FilteredThriftClientModule.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/FilteredThriftClientModule.scala index 5b9d26e6c6..49ca870f23 100644 --- a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/FilteredThriftClientModule.scala +++ b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/FilteredThriftClientModule.scala @@ -63,7 +63,7 @@ object FilteredThriftClientModule { * @see [[https://finagle.github.io/blog/2015/09/10/services-per-endpoint-in-scrooge/ Services-per-endpoint in Scrooge]] * @see [[https://twitter.github.io/scrooge/Finagle.html#creating-a-client Finagle Clients]] */ -@deprecated("Use the com.twitter.inject.thrift.modules.ServicePerEndpointModule", "2018-01-08") +@deprecated("Use the com.twitter.inject.thrift.modules.ThriftMethodBuilderClientModule", "2018-01-08") abstract class FilteredThriftClientModule[ FutureIface <: ThriftService: ClassTag, ServiceIface <: ThriftServiceIface.Filterable[ServiceIface]: ClassTag diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ServicePerEndpointModule.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ServicePerEndpointModule.scala deleted file mode 100644 index cf600dfdbf..0000000000 --- a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ServicePerEndpointModule.scala +++ /dev/null @@ -1,231 +0,0 @@ -package com.twitter.inject.thrift.modules - -import com.google.inject.Provides -import com.twitter.finagle.service.Retries.Budget -import com.twitter.finagle.service.{ReqRep, ResponseClass, ResponseClassifier} -import com.twitter.finagle.stats.StatsReceiver -import com.twitter.finagle.thrift.ClientId -import com.twitter.finagle.thrift.service.{Filterable, MethodPerEndpointBuilder, ServicePerEndpointBuilder} -import com.twitter.finagle.{ThriftMux, thriftmux} -import com.twitter.inject.exceptions.PossiblyRetryable -import com.twitter.inject.thrift.ThriftMethodBuilderFactory -import com.twitter.inject.{Injector, TwitterModule} -import com.twitter.util.{Duration, Monitor, NullMonitor, Return, Throw} -import javax.inject.Singleton - -/** - * Provides bindings for a Scrooge-generated `ServicePerEndpoint` and `MethodPerEndpoint`. The - * `MethodPerEndpoint` is constructed via the [[MethodPerEndpointBuilder]] and is thus implemented - * as a thin wrapper over the `ServicePerEndpoint`. - * - * This [[TwitterModule]] allows users to configure a Scrooge-generated `ServicePerEndpoint` which - * can then be used directly or can be wrapped by a `MethodPerEndpoint`. - * - * Note when applying filters that filter order matters. The order in which filters are applied - * is the order in which requests will flow through to the service and the opposite of the order - * in which responses return. See the [[ThriftMethodBuilderFactory]] for more information. - * - * @tparam ServicePerEndpoint A Scrooge-generated ServicePerEndpoint - * @tparam MethodPerEndpoint A Scrooge-generated MethodPerEndpoint - */ -// TODO: remove deprecated ThriftClientModule and rename this to ThriftClientModule -abstract class ServicePerEndpointModule[ServicePerEndpoint <: Filterable[ServicePerEndpoint], MethodPerEndpoint]( - implicit servicePerEndpointBuilder: ServicePerEndpointBuilder[ServicePerEndpoint], - methodPerEndpointBuilder: MethodPerEndpointBuilder[ServicePerEndpoint, MethodPerEndpoint] -) extends TwitterModule { - - /** - * ThriftMux client label. - * @see [[https://twitter.github.io/finagle/guide/Clients.html#observability Clients Observability]] - */ - val label: String - - /** - * Destination of ThriftMux client. - * @see [[https://twitter.github.io/finagle/guide/Names.html Names and Naming in Finagle]] - */ - val dest: String - - /** - * Configures the session acquisition `timeout` of this client (default: unbounded). - * - * @return a [[Duration]] which represents the acquisition timeout - * @see [[com.twitter.finagle.param.ClientSessionParams.acquisitionTimeout]] - * @see [[https://twitter.github.io/finagle/guide/Clients.html#timeouts-expiration]] - */ - protected def sessionAcquisitionTimeout: Duration = Duration.Top - - /** - * Configures a "global" request `timeout` on the ThriftMux client (default: unbounded). - * Note there is the option to configure request timeouts per-method via configuring the - * [[com.twitter.inject.thrift.ThriftMethodBuilder]] for a given method. When applying - * configuration per-method it is recommended to configure timeouts per-method as well. - * - * However, this exists in the case you want *all* requests to *every* method to have - * the same total timeout. - * - * @return a [[Duration]] which represents the total request timeout - * @see [[com.twitter.finagle.param.CommonParams.withRequestTimeout]] - * @see [[https://twitter.github.io/finagle/guide/Clients.html#timeouts-expiration]] - * @see [[com.twitter.inject.thrift.ThriftMethodBuilder.withTimeoutPerRequest]] - * @see [[com.twitter.finagle.thriftmux.MethodBuilder.withTimeoutPerRequest]] - * @see [[com.twitter.inject.thrift.ThriftMethodBuilder.withTimeoutTotal]] - * @see [[com.twitter.finagle.thriftmux.MethodBuilder.withTimeoutTotal]] - */ - protected def requestTimeout: Duration = Duration.Top - - /** - * Default [[com.twitter.finagle.service.RetryBudget]]. It is highly recommended that budgets - * be shared between all filters that retry or re-queue requests to prevent retry storms. - * - * @return a default [[com.twitter.finagle.service.RetryBudget]] - * @see [[https://twitter.github.io/finagle/guide/Clients.html#retries]] - */ - protected def retryBudget: Budget = Budget.default - - /** - * Function to add a user-defined Monitor. A [[com.twitter.finagle.util.DefaultMonitor]] will be - * installed implicitly which handles all exceptions caught in the stack. Exceptions that are not - * handled by a user-defined monitor are propagated to the [[com.twitter.finagle.util.DefaultMonitor]]. - * - * NullMonitor has no influence on DefaultMonitor behavior here. - */ - protected def monitor: Monitor = NullMonitor - - @Provides - @Singleton - final def providesMethodPerEndpoint( - servicePerEndpoint: ServicePerEndpoint - ): MethodPerEndpoint = { - assert(thriftMuxClient != null, "Unexpected order of initialization.") - thriftMuxClient - .methodPerEndpoint[ServicePerEndpoint, MethodPerEndpoint](servicePerEndpoint) - } - - @Provides - @Singleton - final def providesServicePerEndpoint( - injector: Injector, - clientId: ClientId, - statsReceiver: StatsReceiver - ): ServicePerEndpoint = { - createThriftMuxClient(clientId, statsReceiver) - - val methodBuilder = - configureMethodBuilder( - thriftMuxClient.methodBuilder(dest)) - - configureServicePerEndpoint( - builder = new ThriftMethodBuilderFactory[ServicePerEndpoint]( - injector, - methodBuilder - ), - servicePerEndpoint = thriftMuxClient.servicePerEndpoint(dest, label) - ) - } - - /* Protected */ - - protected val PossiblyRetryableExceptions: ResponseClassifier = - ResponseClassifier.named("PossiblyRetryableExceptions") { - case ReqRep(_, Throw(t)) if PossiblyRetryable.possiblyRetryable(t) => - ResponseClass.RetryableFailure - case ReqRep(_, Throw(_)) => - ResponseClass.NonRetryableFailure - case ReqRep(_, Return(_)) => - ResponseClass.Success - } - - /** - * This method allows for extended configuration of the base MethodBuilder (e.g., the MethodBuilder - * used as a starting point for all method configurations) not exposed by this module or for - * overriding provided defaults, e.g., - * - * {{{ - * override def configureMethodBuilder(methodBuilder: thriftmux.MethodBuilder): thriftmux.MethodBuilder = { - * methodBuilder - * .withTimeoutTotal(5.seconds) - * } - * }}} - * - * Note: any configuration here will be applied to all methods unless explicitly overridden. However, - * also note that filters are cumulative. Thus filters added here will be present in any final configuration. - * - * @param methodBuilder the [[thriftmux.MethodBuilder]] to configure. - * @return a configured MethodBuilder which will be used as the starting point for any per-method - * configuration. - */ - protected def configureMethodBuilder( - methodBuilder: thriftmux.MethodBuilder - ): thriftmux.MethodBuilder = methodBuilder - - /** - * This method allows for further configuration of the ThriftMux client for parameters not exposed by - * this module or for overriding defaults provided herein, e.g., - * - * {{{ - * override def configureThriftMuxClient(client: ThriftMux.Client): ThriftMux.Client = { - * client - * .withProtocolFactory(myCustomProtocolFactory)) - * .withStatsReceiver(someOtherScopedStatsReceiver) - * .withMonitor(myAwesomeMonitor) - * .withTracer(notTheDefaultTracer) - * .withResponseClassifier(ThriftResponseClassifier.ThriftExceptionsAsFailures) - * } - *}}} - * - * @param client the [[com.twitter.finagle.ThriftMux.Client]] to configure. - * @return a configured ThriftMux.Client. - */ - protected def configureThriftMuxClient( - client: ThriftMux.Client - ): ThriftMux.Client = client - - /** - * Configure the ServicePerEndpoint. This is done by using the given [[ThriftMethodBuilderFactory]] - * to configure a [[com.twitter.inject.thrift.ThriftMethodBuilder]] for a given ThriftMethod. E.g., - * - * servicePerEndpoint - * .withFetchBlob( - * builder.method(FetchBlob) - * ... - * - * Subclasses of this module MAY provide an implementation of `configureServicePerEndpoint` which - * specifies configuration of a `ServicePerEndpoint` interface per-method of the interface. - * - * @param builder a [[ThriftMethodBuilderFactory]] for creating a [[com.twitter.inject.thrift.ThriftMethodBuilder]]. - * @param servicePerEndpoint the [[ServicePerEndpoint]] to configure. - * @return a per-method filtered [[ServicePerEndpoint]] - * @see [[com.twitter.inject.thrift.ThriftMethodBuilder]] - */ - protected def configureServicePerEndpoint( - builder: ThriftMethodBuilderFactory[ServicePerEndpoint], - servicePerEndpoint: ServicePerEndpoint - ): ServicePerEndpoint = servicePerEndpoint - - /* Private */ - - // We want each module to be able to configure a ThriftMux.client independently, - // however we do not want the client to be exposed in the object graph as including - // multiple modules in a server would attempt to bind the same type multiple times - // which would fail. Thus we use mutation to create and configure a ThriftMux.Client. - private[this] var thriftMuxClient: ThriftMux.Client = _ - private[this] def createThriftMuxClient( - clientId: ClientId, - statsReceiver: StatsReceiver - ): Unit = { - val clientStatsReceiver = statsReceiver.scope("clnt") - - thriftMuxClient = configureThriftMuxClient( - ThriftMux.client.withSession - .acquisitionTimeout(sessionAcquisitionTimeout) - .withRequestTimeout(requestTimeout) - .withStatsReceiver(clientStatsReceiver) - .withClientId(clientId) - .withMonitor(monitor) - .withLabel(label) - .withRetryBudget(retryBudget.retryBudget) - .withRetryBackoff(retryBudget.requeueBackoffs) - ) - } -} diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftClientIdModule.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftClientIdModule.scala index 96e0d5378e..7ddcdf4d57 100644 --- a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftClientIdModule.scala +++ b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftClientIdModule.scala @@ -7,6 +7,12 @@ import javax.inject.Singleton object ThriftClientIdModule extends ThriftClientIdModule +/** + * Provides a [[com.twitter.finagle.thrift.ClientId]] binding with a value + * configurable via a [[com.twitter.app.Flag]]. + * + * @see [[https://twitter.github.io/finatra/user-guide/getting-started/flags.html Flags]] + */ class ThriftClientIdModule extends TwitterModule { private val clientIdFlag = flag("thrift.clientId", "", "Thrift client id") diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftClientModule.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftClientModule.scala index 9b9d0be32a..908d8f1239 100644 --- a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftClientModule.scala +++ b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftClientModule.scala @@ -1,65 +1,69 @@ package com.twitter.inject.thrift.modules -import com.github.nscala_time.time import com.google.inject.Provides import com.twitter.finagle._ -import com.twitter.finagle.factory.TimeoutFactory -import com.twitter.finagle.param.Stats -import com.twitter.finagle.service.TimeoutFilter +import com.twitter.finagle.service.Retries.Budget import com.twitter.finagle.stats.StatsReceiver import com.twitter.finagle.thrift.ClientId import com.twitter.inject.TwitterModule -import com.twitter.util.Duration +import com.twitter.util.{Duration, Monitor, NullMonitor} import javax.inject.Singleton import scala.reflect.ClassTag -@deprecated("Use the com.twitter.inject.thrift.modules.ServicePerEndpointModule", "2018-01-12") -abstract class ThriftClientModule[T: ClassTag] extends TwitterModule with time.Implicits { +/** + * A [[TwitterModule]] allows users to configure a Finagle `ThriftMux` client and does NOT + * provide ability to filter or configure per-method Scrooge-generated interfaces. The client + * interface can be expressed as a `MethodPerEndpoint` or higher-kinded interface. + * + * Provides bindings for a Scrooge-generated `MethodPerEndpoint` or higher-kinded interface. + * + * See the [[ThriftMethodBuilderClientModule]] for building a `ThriftMux` client that allows for filtering + * and configuration per-method of the Scrooge-generated interface. + * + * @note This [[TwitterModule]] expects a [[com.twitter.finagle.thrift.ClientId]] to be bound to + * the object graph but does not assume how it is done. A [[com.twitter.finagle.thrift.ClientId]] + * can be bound by including the [[ThriftClientIdModule]] in your server configuration. + * + * @tparam ThriftService A Scrooge-generated `MethodPerEndpoint` or the higher-kinded type of the + * Scrooge-generated service, e.g., `MyService[Future]`. + * @see [[com.twitter.finagle.thrift.ThriftRichClient.build(dest: String, label: String)]] + * @see [[https://twitter.github.io/finagle/guide/Clients.html Finagle Clients]] + * @see [[https://twitter.github.io/finagle/guide/FAQ.html?highlight=thriftmux#what-is-thriftmux What is ThriftMux?]] + */ +abstract class ThriftClientModule[ThriftService: ClassTag] + extends TwitterModule + with ThriftClientModuleTrait { - /** - * Name of client for use in metrics - */ - val label: String + protected def sessionAcquisitionTimeout: Duration = Duration.Top - /** - * Destination of client - */ - val dest: String + protected def requestTimeout: Duration = Duration.Top - /** - * Enable thrift mux for this connection. - * - * Note: Both server and client must have mux enabled otherwise - * a nondescript ChannelClosedException will be seen. - * - * What is ThriftMux? - * https://twitter.github.io/finagle/guide/FAQ.html?highlight=thriftmux#what-is-thriftmux - */ - def mux: Boolean = true + protected def retryBudget: Budget = Budget.default - def requestTimeout: Duration = Duration.Top + protected def monitor: Monitor = NullMonitor - def connectTimeout: Duration = Duration.Top + protected def configureThriftMuxClient( + client: ThriftMux.Client + ): ThriftMux.Client = client @Singleton @Provides - def providesClient(clientId: ClientId, statsReceiver: StatsReceiver): T = { - val labelAndDest = s"$label=$dest" + final def providesThriftClient( + clientId: ClientId, + statsReceiver: StatsReceiver + ): ThriftService = { + val clientStatsReceiver = statsReceiver.scope("clnt") - if (mux) { - ThriftMux.client - .configured(TimeoutFilter.Param(requestTimeout)) - .configured(TimeoutFactory.Param(connectTimeout)) - .configured(Stats(statsReceiver.scope("clnt"))) + configureThriftMuxClient( + ThriftMux.client.withSession + .acquisitionTimeout(sessionAcquisitionTimeout) + .withRequestTimeout(requestTimeout) + .withStatsReceiver(clientStatsReceiver) .withClientId(clientId) - .newIface[T](labelAndDest) - } else { - Thrift.client - .configured(TimeoutFilter.Param(requestTimeout)) - .configured(TimeoutFactory.Param(connectTimeout)) - .configured(Stats(statsReceiver.scope("clnt"))) - .withClientId(clientId) - .newIface[T](labelAndDest) - } + .withMonitor(monitor) + .withLabel(label) + .withRetryBudget(retryBudget.retryBudget) + .withRetryBackoff(retryBudget.requeueBackoffs) + ).build[ThriftService](dest, label) } } diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftClientModuleTrait.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftClientModuleTrait.scala new file mode 100644 index 0000000000..53a38efd84 --- /dev/null +++ b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftClientModuleTrait.scala @@ -0,0 +1,79 @@ +package com.twitter.inject.thrift.modules + +import com.twitter.finagle.ThriftMux +import com.twitter.finagle.service.Retries.Budget +import com.twitter.util.{Duration, Monitor} + +private[inject] trait ThriftClientModuleTrait { + + /** + * ThriftMux client label. + * @see [[https://twitter.github.io/finagle/guide/Clients.html#observability Clients Observability]] + */ + def label: String + + /** + * Destination of ThriftMux client. + * @see [[https://twitter.github.io/finagle/guide/Names.html Names and Naming in Finagle]] + */ + def dest: String + + /** + * Configures the session acquisition `timeout` of this client (default: unbounded). + * + * @return a [[Duration]] which represents the acquisition timeout + * @see [[com.twitter.finagle.param.ClientSessionParams.acquisitionTimeout]] + * @see [[https://twitter.github.io/finagle/guide/Clients.html#timeouts-expiration]] + */ + protected def sessionAcquisitionTimeout: Duration + + /** + * Configures a "global" request `timeout` on the ThriftMux client (default: unbounded). + * This will set *all* requests to *every* method to have the same total timeout. + * + * @return a [[Duration]] which represents the total request timeout + * @see [[com.twitter.finagle.param.CommonParams.withRequestTimeout]] + * @see [[https://twitter.github.io/finagle/guide/Clients.html#timeouts-expiration]] + */ + protected def requestTimeout: Duration + + /** + * Default [[com.twitter.finagle.service.RetryBudget]]. It is highly recommended that budgets + * be shared between all filters that retry or re-queue requests to prevent retry storms. + * + * @return a default [[com.twitter.finagle.service.RetryBudget]] + * @see [[https://twitter.github.io/finagle/guide/Clients.html#retries]] + */ + protected def retryBudget: Budget + + /** + * Function to add a user-defined Monitor. A [[com.twitter.finagle.util.DefaultMonitor]] will be + * installed implicitly which handles all exceptions caught in the stack. Exceptions that are not + * handled by a user-defined monitor are propagated to the [[com.twitter.finagle.util.DefaultMonitor]]. + * + * NullMonitor has no influence on DefaultMonitor behavior here. + */ + protected def monitor: Monitor + + /** + * This method allows for further configuration of the ThriftMux client for parameters not exposed by + * this module or for overriding defaults provided herein, e.g., + * + * {{{ + * override def configureThriftMuxClient(client: ThriftMux.Client): ThriftMux.Client = { + * client + * .withProtocolFactory(myCustomProtocolFactory)) + * .withStatsReceiver(someOtherScopedStatsReceiver) + * .withMonitor(myAwesomeMonitor) + * .withTracer(notTheDefaultTracer) + * .withResponseClassifier(ThriftResponseClassifier.ThriftExceptionsAsFailures) + * } + *}}} + * + * @param client the [[com.twitter.finagle.ThriftMux.Client]] to configure. + * @return a configured [[ThriftMux.Client]]. + */ + protected def configureThriftMuxClient( + client: ThriftMux.Client + ): ThriftMux.Client +} diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftMethodBuilderClientModule.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftMethodBuilderClientModule.scala new file mode 100644 index 0000000000..bbe50fc29f --- /dev/null +++ b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/ThriftMethodBuilderClientModule.scala @@ -0,0 +1,161 @@ +package com.twitter.inject.thrift.modules + +import com.google.inject.Provides +import com.twitter.finagle.service.Retries.Budget +import com.twitter.finagle.stats.StatsReceiver +import com.twitter.finagle.thrift.ClientId +import com.twitter.finagle.thrift.service.{Filterable, MethodPerEndpointBuilder, ServicePerEndpointBuilder} +import com.twitter.finagle.{ThriftMux, thriftmux} +import com.twitter.inject.thrift.ThriftMethodBuilderFactory +import com.twitter.inject.{Injector, TwitterModule} +import com.twitter.util.{Duration, Monitor, NullMonitor} +import javax.inject.Singleton + +/** + * A [[TwitterModule]] which allows for configuration of a `ThriftMux` client. The client interface + * can be expressed as a `service-per-endpoint` or a `MethodPerEndpoint`. + * + * Provides bindings for a Scrooge-generated `service-per-endpoint` and `MethodPerEndpoint`. The + * `MethodPerEndpoint` is constructed via the [[MethodPerEndpointBuilder]] and is thus implemented + * as a thin wrapper over the `service-per-endpoint`. + * + * This [[TwitterModule]] allows users to configure and filter a Scrooge-generated `service-per-endpoint` + * per-method which can then be used directly or can be wrapped by a `MethodPerEndpoint`. + * + * @note When applying filters, filter order matters. The order in which filters are applied + * is the order in which requests will flow through to the service and the opposite of the + * order in which responses return. See the [[ThriftMethodBuilderFactory]] for more information. + * + * @note This [[TwitterModule]] expects a [[com.twitter.finagle.thrift.ClientId]] to be bound to + * the object graph but does not assume how it is done. A [[com.twitter.finagle.thrift.ClientId]] + * can be bound by including the [[ThriftClientIdModule]] in your server configuration. + * + * @tparam ServicePerEndpoint A Scrooge-generated ServicePerEndpoint + * @tparam MethodPerEndpoint A Scrooge-generated MethodPerEndpoint + * @see [[https://twitter.github.io/scrooge/Finagle.html#id1 MethodPerEndpoint]] + * @see [[https://twitter.github.io/scrooge/Finagle.html#id2 ServicePerEndpoint]] + * @see [[https://twitter.github.io/scrooge/Finagle.html#id3 ReqRepServicePerEndpoint]] + * @see [[https://twitter.github.io/finagle/guide/Clients.html Finagle Clients]] + * @see [[https://twitter.github.io/finagle/guide/FAQ.html?highlight=thriftmux#what-is-thriftmux What is ThriftMux?]] + */ +abstract class ThriftMethodBuilderClientModule[ServicePerEndpoint <: Filterable[ServicePerEndpoint], MethodPerEndpoint]( + implicit servicePerEndpointBuilder: ServicePerEndpointBuilder[ServicePerEndpoint], + methodPerEndpointBuilder: MethodPerEndpointBuilder[ServicePerEndpoint, MethodPerEndpoint] +) extends TwitterModule + with ThriftClientModuleTrait { + + protected def sessionAcquisitionTimeout: Duration = Duration.Top + + protected def requestTimeout: Duration = Duration.Top + + protected def retryBudget: Budget = Budget.default + + protected def monitor: Monitor = NullMonitor + + protected def configureThriftMuxClient( + client: ThriftMux.Client + ): ThriftMux.Client = client + + /** + * This method allows for extended configuration of the base MethodBuilder (e.g., the MethodBuilder + * used as a starting point for all method configurations) not exposed by this module or for + * overriding provided defaults, e.g., + * + * {{{ + * override def configureMethodBuilder(methodBuilder: thriftmux.MethodBuilder): thriftmux.MethodBuilder = { + * methodBuilder + * .withTimeoutTotal(5.seconds) + * } + * }}} + * + * Note: any configuration here will be applied to all methods unless explicitly overridden. However, + * also note that filters are cumulative. Thus filters added here will be present in any final configuration. + * + * @param methodBuilder the [[thriftmux.MethodBuilder]] to configure. + * @return a configured MethodBuilder which will be used as the starting point for any per-method + * configuration. + */ + protected def configureMethodBuilder( + methodBuilder: thriftmux.MethodBuilder + ): thriftmux.MethodBuilder = methodBuilder + + /** + * Configure the ServicePerEndpoint. This is done by using the given [[ThriftMethodBuilderFactory]] + * to configure a [[com.twitter.inject.thrift.ThriftMethodBuilder]] for a given ThriftMethod. E.g., + * + * servicePerEndpoint + * .withFetchBlob( + * builder.method(FetchBlob) + * ... + * + * Subclasses of this module MAY provide an implementation of `configureServicePerEndpoint` which + * specifies configuration of a `ServicePerEndpoint` interface per-method of the interface. + * + * @param builder a [[ThriftMethodBuilderFactory]] for creating a [[com.twitter.inject.thrift.ThriftMethodBuilder]]. + * @param servicePerEndpoint the [[ServicePerEndpoint]] to configure. + * @return a per-method filtered [[ServicePerEndpoint]] + * @see [[com.twitter.inject.thrift.ThriftMethodBuilder]] + */ + protected def configureServicePerEndpoint( + builder: ThriftMethodBuilderFactory[ServicePerEndpoint], + servicePerEndpoint: ServicePerEndpoint + ): ServicePerEndpoint = servicePerEndpoint + + @Provides + @Singleton + final def providesMethodPerEndpoint( + servicePerEndpoint: ServicePerEndpoint + ): MethodPerEndpoint = { + assert(thriftMuxClient != null, "Unexpected order of initialization.") + thriftMuxClient + .methodPerEndpoint[ServicePerEndpoint, MethodPerEndpoint](servicePerEndpoint) + } + + @Provides + @Singleton + final def providesServicePerEndpoint( + injector: Injector, + clientId: ClientId, + statsReceiver: StatsReceiver + ): ServicePerEndpoint = { + createThriftMuxClient(clientId, statsReceiver) + + val methodBuilder = + configureMethodBuilder(thriftMuxClient.methodBuilder(dest)) + + configureServicePerEndpoint( + builder = new ThriftMethodBuilderFactory[ServicePerEndpoint]( + injector, + methodBuilder + ), + servicePerEndpoint = thriftMuxClient.servicePerEndpoint(dest, label) + ) + } + + /* Private */ + + // We want each module to be able to configure a ThriftMux.client independently + // and it is needed by the instances to be exposed to the object graph, however we do + // not want the client to be exposed in the object graph as including multiple modules + // in a server would attempt to bind the same type multiple times which would error. + // Thus we use mutation to create and configure a ThriftMux.Client. + private[this] var thriftMuxClient: ThriftMux.Client = _ + private[this] def createThriftMuxClient( + clientId: ClientId, + statsReceiver: StatsReceiver + ): Unit = { + val clientStatsReceiver = statsReceiver.scope("clnt") + + thriftMuxClient = configureThriftMuxClient( + ThriftMux.client.withSession + .acquisitionTimeout(sessionAcquisitionTimeout) + .withRequestTimeout(requestTimeout) + .withStatsReceiver(clientStatsReceiver) + .withClientId(clientId) + .withMonitor(monitor) + .withLabel(label) + .withRetryBudget(retryBudget.retryBudget) + .withRetryBackoff(retryBudget.requeueBackoffs) + ) + } +} diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/package.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/package.scala new file mode 100644 index 0000000000..02e3ba6a34 --- /dev/null +++ b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/package.scala @@ -0,0 +1,19 @@ +package com.twitter.inject.thrift + +import com.twitter.finagle.service.{ReqRep, ResponseClass, ResponseClassifier} +import com.twitter.inject.exceptions.PossiblyRetryable +import com.twitter.util.{Return, Throw} + +package object modules { + + val PossiblyRetryableExceptions: ResponseClassifier = + ResponseClassifier.named("PossiblyRetryableExceptions") { + case ReqRep(_, Throw(t)) if PossiblyRetryable.possiblyRetryable(t) => + ResponseClass.RetryableFailure + case ReqRep(_, Throw(_)) => + ResponseClass.NonRetryableFailure + case ReqRep(_, Return(_)) => + ResponseClass.Success + } + +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingReqRepServicePerEndpointModuleFeatureTest.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingReqRepThriftMethodBuilderClientModuleFeatureTest.scala similarity index 91% rename from inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingReqRepServicePerEndpointModuleFeatureTest.scala rename to inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingReqRepThriftMethodBuilderClientModuleFeatureTest.scala index e2af474699..1b945572f7 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingReqRepServicePerEndpointModuleFeatureTest.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingReqRepThriftMethodBuilderClientModuleFeatureTest.scala @@ -6,18 +6,18 @@ import com.twitter.finatra.http.{EmbeddedHttpServer, HttpTest} import com.twitter.finatra.thrift.EmbeddedThriftServer import com.twitter.greeter.thriftscala.Greeter import com.twitter.inject.server.FeatureTest -import com.twitter.inject.thrift.DoEverythingReqRepServicePerEndpointModuleFeatureTest._ -import com.twitter.inject.thrift.integration.reqrepserviceperendpoint.{ReqRepServicePerEndpointHttpController, GreeterReqRepServicePerEndpointModule, GreeterThriftService} +import com.twitter.inject.thrift.DoEverythingReqRepThriftMethodBuilderClientModuleFeatureTest._ +import com.twitter.inject.thrift.integration.reqrepserviceperendpoint.{ReqRepServicePerEndpointHttpController, GreeterReqRepThriftMethodBuilderClientModule, GreeterThriftService} import com.twitter.inject.thrift.integration.{TestHttpServer, TestThriftServer} import com.twitter.util.Duration import com.twitter.util.tunable.Tunable -object DoEverythingReqRepServicePerEndpointModuleFeatureTest { +object DoEverythingReqRepThriftMethodBuilderClientModuleFeatureTest { case class HelloHeaders(empty: Boolean) case class HelloResponse(value: String, headers: HelloHeaders) } -class DoEverythingReqRepServicePerEndpointModuleFeatureTest +class DoEverythingReqRepThriftMethodBuilderClientModuleFeatureTest extends FeatureTest with HttpTest { override val printStats = false @@ -36,7 +36,7 @@ class DoEverythingReqRepServicePerEndpointModuleFeatureTest override val server = new EmbeddedHttpServer( twitterServer = new TestHttpServer[ReqRepServicePerEndpointHttpController]( "rrspe-server", - new GreeterReqRepServicePerEndpointModule(requestHeaderKey, perRequestTimeoutTunable)), + new GreeterReqRepThriftMethodBuilderClientModule(requestHeaderKey, perRequestTimeoutTunable)), args = Seq( s"-thrift.clientId=$httpServiceClientId", resolverMap( diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingThriftClientModuleFeatureTest.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingThriftClientModuleFeatureTest.scala index 72c17aa473..02c73af6d9 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingThriftClientModuleFeatureTest.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingThriftClientModuleFeatureTest.scala @@ -3,22 +3,20 @@ package com.twitter.inject.thrift import com.twitter.conversions.time._ import com.twitter.finagle.http.Request import com.twitter.finagle.http.Status._ -import com.twitter.finagle.thrift.ClientId -import com.twitter.finagle.{ListeningServer, ServiceClosedException, ThriftMux} -import com.twitter.finatra.http.filters.CommonFilters -import com.twitter.finatra.http.routing.HttpRouter -import com.twitter.finatra.http.{Controller, EmbeddedHttpServer, HttpServer, HttpTest} +import com.twitter.finagle.ServiceClosedException +import com.twitter.finatra.http.{Controller, EmbeddedHttpServer, HttpTest} import com.twitter.finatra.thrift.EmbeddedThriftServer -import com.twitter.inject.server.{PortUtils, TwitterServer} +import com.twitter.inject.server.FeatureTest import com.twitter.inject.thrift.DoEverythingThriftClientModuleFeatureTest._ -import com.twitter.inject.thrift.modules.{ThriftClientIdModule, ThriftClientModule} -import com.twitter.inject.{Logging, Test} +import com.twitter.inject.thrift.integration.basic.{EchoThriftClientModule1, EchoThriftClientModule2, EchoThriftClientModule3, MyEchoService} +import com.twitter.inject.thrift.integration.{TestHttpServer, TestThriftServer} import com.twitter.test.thriftscala.EchoService import com.twitter.util.{Await, Future} -import java.util.concurrent.atomic.AtomicInteger import javax.inject.{Inject, Singleton} object DoEverythingThriftClientModuleFeatureTest { + @Singleton class EchoServiceFuture @Inject()(val service: EchoService[Future]) + class EchoHttpController1 @Inject()(echoThriftService: EchoService[Future]) extends Controller { get("/echo") { request: Request => val msg = request.params("msg") @@ -30,7 +28,9 @@ object DoEverythingThriftClientModuleFeatureTest { } } - class EchoHttpController2 @Inject()(echoThriftService: EchoService.FutureIface) extends Controller { + /* Testing deprecated code for coverage */ + class EchoHttpController2 @Inject()(echoThriftService: EchoService.FutureIface) + extends Controller { get("/echo") { request: Request => val msg = request.params("msg") echoThriftService.echo(msg) @@ -41,111 +41,69 @@ object DoEverythingThriftClientModuleFeatureTest { } } - object EchoThriftClientModule1 - extends ThriftClientModule[EchoService[Future]] { - override val label = "echo-service" - override val dest = "flag!thrift-echo-service" - } - - object EchoThriftClientModule2 - extends ThriftClientModule[EchoService.FutureIface] { - override val label = "echo-service" - override val dest = "flag!thrift-echo-service" - } - - @Singleton - class MyEchoService extends EchoService[Future] with Logging { - private val timesToEcho = new AtomicInteger(1) - - /* Public */ - override def echo(msg: String): Future[String] = { - info("echo " + msg) - assertClientId("echo-http-service") - Future.value(msg * timesToEcho.get) - } - override def setTimesToEcho(times: Int): Future[Int] = { - info("setTimesToEcho " + times) - assertClientId("echo-http-service") - timesToEcho.set(times) //mutation - Future(times) - } - - /* Private */ - private def assertClientId(name: String): Unit = { - assert(ClientId.current.contains(ClientId(name)), "Invalid Client ID: " + ClientId.current) - } - } - - class EchoThriftServer extends TwitterServer { - private val thriftPortFlag = flag("thrift.port", ":0", "External Thrift server port") - private val thriftShutdownTimeout = flag( - "thrift.shutdown.time", - 1.minute, - "Maximum amount of time to wait for pending requests to complete on shutdown" - ) - /* Private Mutable State */ - private var thriftServer: ListeningServer = _ - - /* Lifecycle */ - override def postWarmup() { - super.postWarmup() - thriftServer = ThriftMux.server.serveIface(thriftPortFlag(), injector.instance[MyEchoService]) - info("Thrift server started on port: " + thriftPort.get) + class EchoHttpController3 @Inject()(echoThriftService: EchoService.MethodPerEndpoint) + extends Controller { + get("/echo") { request: Request => + val msg = request.params("msg") + echoThriftService.echo(msg) } - - onExit { - Await.result(thriftServer.close(thriftShutdownTimeout().fromNow)) + post("/config") { request: Request => + val timesToEcho = request.params("timesToEcho").toInt + echoThriftService.setTimesToEcho(timesToEcho) } - - /* Overrides */ - override def thriftPort = Option(thriftServer) map PortUtils.getPort } } -class DoEverythingThriftClientModuleFeatureTest extends Test with HttpTest { +class DoEverythingThriftClientModuleFeatureTest extends FeatureTest with HttpTest { + override val printStats = false private val clientIdString = "echo-http-service" - val thriftServer = - new EmbeddedThriftServer( - twitterServer = new EchoThriftServer) - - val httpServer1 = new EmbeddedHttpServer( - twitterServer = new HttpServer { - override val name = "echo-http-server1" - override val modules = Seq(ThriftClientIdModule, EchoThriftClientModule1) - override def configureHttp(router: HttpRouter) { - router.filter[CommonFilters].add[EchoHttpController1] - } - }, + override val server = + new EmbeddedThriftServer(twitterServer = new TestThriftServer(new MyEchoService)) + + private val httpServer1 = new EmbeddedHttpServer( + twitterServer = + new TestHttpServer[EchoHttpController1]("echo-http-server1", EchoThriftClientModule1), args = Seq( s"-thrift.clientId=$clientIdString", - resolverMap("thrift-echo-service" -> thriftServer.thriftHostAndPort) + resolverMap("thrift-echo-service" -> server.thriftHostAndPort) ) ) - val httpServer2 = new EmbeddedHttpServer( - twitterServer = new HttpServer { - override val name = "echo-http-server2" - override val modules = Seq(ThriftClientIdModule, EchoThriftClientModule2) - override def configureHttp(router: HttpRouter) { - router.filter[CommonFilters].add[EchoHttpController2] - } - }, + private val httpServer2 = new EmbeddedHttpServer( + twitterServer = + new TestHttpServer[EchoHttpController2]("echo-http-server2", EchoThriftClientModule2), args = Seq( - "-thrift.clientId=echo-http-service", - resolverMap("thrift-echo-service" -> thriftServer.thriftHostAndPort) + s"-thrift.clientId=$clientIdString", + resolverMap("thrift-echo-service" -> server.thriftHostAndPort) + ) + ) + + private val httpServer3 = new EmbeddedHttpServer( + twitterServer = + new TestHttpServer[EchoHttpController3]("echo-http-server3", EchoThriftClientModule3), + args = Seq( + s"-thrift.clientId=$clientIdString", + resolverMap("thrift-echo-service" -> server.thriftHostAndPort) ) ) override def afterAll(): Unit = { - thriftServer.close() httpServer1.close() httpServer2.close() + httpServer3.close() super.afterAll() } + private def await[T](f: Future[T]): T = { + Await.result(f, 5.seconds) + } + // test EchoService[Future] + test("EchoService[Future] is available from the injector") { + httpServer1.injector.instance[EchoServiceFuture].service should not be null + } test("EchoHttpServer1#echo 3 times") { httpServer1.httpPost( path = "/config?timesToEcho=2", @@ -167,6 +125,9 @@ class DoEverythingThriftClientModuleFeatureTest extends Test with HttpTest { } // test EchoService.FutureIface + test("EchoService.FutureIface is available from the injector") { + httpServer2.injector.instance[EchoService.FutureIface] should not be null + } test("EchoHttpServer2#echo 3 times") { httpServer2.httpPost( path = "/config?timesToEcho=2", @@ -187,18 +148,42 @@ class DoEverythingThriftClientModuleFeatureTest extends Test with HttpTest { httpServer2.assertStat("route/echo/GET/response_size", Seq(9)) } - test("EchoThriftServer#echo 3 times") { - val thriftClient = thriftServer + // test EchoService.MethodPerEndpoint + test("EchoService.MethodPerEndpoint is available from the injector") { + httpServer3.injector.instance[EchoService.MethodPerEndpoint] should not be null + } + test("EchoHttpServer3#echo 3 times") { + httpServer3.httpPost( + path = "/config?timesToEcho=2", + postBody = "", + andExpect = Ok, + withBody = "2" + ) + + httpServer3.httpPost( + path = "/config?timesToEcho=3", + postBody = "", + andExpect = Ok, + withBody = "3" + ) + + httpServer3.httpGet(path = "/echo?msg=Bob", andExpect = Ok, withBody = "BobBobBob") + httpServer3.assertStat("route/config/POST/response_size", Seq(1, 1)) + httpServer3.assertStat("route/echo/GET/response_size", Seq(9)) + } + + test("ThriftClient#asClosable") { + val thriftClient = server .thriftClient[EchoService[Future]](clientIdString) - Await.result(thriftClient.setTimesToEcho(2), 5.seconds) - Await.result(thriftClient.setTimesToEcho(3), 5.seconds) + await(thriftClient.setTimesToEcho(2)) + await(thriftClient.setTimesToEcho(3)) - assert(Await.result(thriftClient.echo("Bob"), 5.seconds) == "BobBobBob") + assertFutureValue(thriftClient.echo("Bob"), "BobBobBob") - Await.result(thriftClient.asClosable.close(), 5.seconds) + await(thriftClient.asClosable.close()) intercept[ServiceClosedException] { - Await.result(thriftClient.echo("Bob"), 5.seconds) + await(thriftClient.echo("Bob")) } } } diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingServicePerEndpointModuleFeatureTest.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingThriftMethodBuilderClientModuleFeatureTest.scala similarity index 96% rename from inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingServicePerEndpointModuleFeatureTest.scala rename to inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingThriftMethodBuilderClientModuleFeatureTest.scala index 742ce6dcc6..d39d219486 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingServicePerEndpointModuleFeatureTest.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/DoEverythingThriftMethodBuilderClientModuleFeatureTest.scala @@ -6,16 +6,16 @@ import com.twitter.finatra.thrift.EmbeddedThriftServer import com.twitter.greeter.thriftscala.Greeter import com.twitter.inject.server.FeatureTest import com.twitter.inject.thrift.integration.serviceperendpoint.{ - EchoServicePerEndpointModule, + EchoThriftMethodBuilderClientModule, EchoThriftService, - GreeterServicePerEndpointModule, + GreeterThriftMethodBuilderClientModule, GreeterThriftService, ServicePerEndpointHttpController } import com.twitter.inject.thrift.integration.{TestHttpServer, TestThriftServer} import com.twitter.test.thriftscala.EchoService -class DoEverythingServicePerEndpointModuleFeatureTest extends FeatureTest with HttpTest { +class DoEverythingThriftMethodBuilderClientModuleFeatureTest extends FeatureTest with HttpTest { override val printStats = false private val httpServiceClientId = "http-service" @@ -33,8 +33,8 @@ class DoEverythingServicePerEndpointModuleFeatureTest extends FeatureTest with H val server = new EmbeddedHttpServer( twitterServer = new TestHttpServer[ServicePerEndpointHttpController]( "spe-server", - GreeterServicePerEndpointModule, - EchoServicePerEndpointModule + GreeterThriftMethodBuilderClientModule, + EchoThriftMethodBuilderClientModule ), args = Seq( s"-thrift.clientId=$httpServiceClientId", diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/InheritanceServicePerEndpointModuleFeatureTest.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/InheritanceThriftMethodBuilderClientModuleFeatureTest.scala similarity index 98% rename from inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/InheritanceServicePerEndpointModuleFeatureTest.scala rename to inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/InheritanceThriftMethodBuilderClientModuleFeatureTest.scala index 5c8587b179..b217566115 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/InheritanceServicePerEndpointModuleFeatureTest.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/InheritanceThriftMethodBuilderClientModuleFeatureTest.scala @@ -8,7 +8,7 @@ import com.twitter.inject.thrift.integration.TestThriftServer import com.twitter.inject.thrift.integration.inheritance.{ServiceBHttpServer, ServiceBThriftService} import com.twitter.serviceB.thriftscala.ServiceB -class InheritanceServicePerEndpointModuleFeatureTest +class InheritanceThriftMethodBuilderClientModuleFeatureTest extends FeatureTest with HttpTest { override val printStats = false diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/ThriftClientModuleNonMuxTest.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/ThriftClientModuleNonMuxTest.scala deleted file mode 100644 index 7447a0103d..0000000000 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/ThriftClientModuleNonMuxTest.scala +++ /dev/null @@ -1,27 +0,0 @@ -package com.twitter.inject.thrift - -import com.twitter.greeter.thriftscala.Greeter -import com.twitter.inject.Test -import com.twitter.inject.app.TestInjector -import com.twitter.inject.modules.StatsReceiverModule -import com.twitter.inject.thrift.modules.{ThriftClientIdModule, ThriftClientModule} - -class ThriftClientModuleNonMuxTest extends Test { - - val injector = - TestInjector( - modules = Seq(ThriftClientModuleNonMux, ThriftClientIdModule, StatsReceiverModule), - flags = Map("com.twitter.server.resolverMap" -> "greeter-thrift-service=nil!") - ).create - - test("client is created as expected") { - val client = injector.instance[Greeter.FutureIface] - assert(client != null) - } - - object ThriftClientModuleNonMux extends ThriftClientModule[Greeter.FutureIface] { - override val label = "greeter-thrift-client" - override val dest = "flag!greeter-thrift-service" - override val mux = false - } -} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/TestThriftServer.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/TestThriftServer.scala index b0dc2df67f..d7fa5d1b09 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/TestThriftServer.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/TestThriftServer.scala @@ -1,5 +1,6 @@ package com.twitter.inject.thrift.integration +import com.twitter.conversions.time._ import com.twitter.finagle.{ListeningServer, ThriftMux} import com.twitter.inject.server.{PortUtils, TwitterServer} import com.twitter.scrooge.ThriftService @@ -7,6 +8,12 @@ import com.twitter.util.Await class TestThriftServer(service: ThriftService) extends TwitterServer { private val thriftPortFlag = flag("thrift.port", ":0", "External Thrift server port") + private val thriftShutdownTimeout = flag( + "thrift.shutdown.time", + 1.minute, + "Maximum amount of time to wait for pending requests to complete on shutdown" + ) + /* Private Mutable State */ private var thriftServer: ListeningServer = _ @@ -18,9 +25,9 @@ class TestThriftServer(service: ThriftService) extends TwitterServer { } onExit { - Await.result(thriftServer.close()) + Await.result(thriftServer.close(thriftShutdownTimeout().fromNow)) } /* Overrides */ - override def thriftPort = Option(thriftServer) map PortUtils.getPort + override def thriftPort: Option[Int] = Option(thriftServer) map PortUtils.getPort } diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/EchoThriftClientModule1.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/EchoThriftClientModule1.scala new file mode 100644 index 0000000000..9e65705a29 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/EchoThriftClientModule1.scala @@ -0,0 +1,18 @@ +package com.twitter.inject.thrift.integration.basic + +import com.twitter.finagle.ThriftMux +import com.twitter.inject.thrift.modules.{PossiblyRetryableExceptions, ThriftClientModule} +import com.twitter.test.thriftscala.EchoService +import com.twitter.util.Future + +object EchoThriftClientModule1 extends ThriftClientModule[EchoService[Future]] { + override val label = "echo-service" + override val dest = "flag!thrift-echo-service" + + override def configureThriftMuxClient( + client: ThriftMux.Client + ): ThriftMux.Client = { + client + .withResponseClassifier(PossiblyRetryableExceptions) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/EchoThriftClientModule2.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/EchoThriftClientModule2.scala new file mode 100644 index 0000000000..9062cbec9f --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/EchoThriftClientModule2.scala @@ -0,0 +1,10 @@ +package com.twitter.inject.thrift.integration.basic + +import com.twitter.inject.thrift.modules.ThriftClientModule +import com.twitter.test.thriftscala.EchoService + +object EchoThriftClientModule2 + extends ThriftClientModule[EchoService.FutureIface] { + override val label = "echo-service" + override val dest = "flag!thrift-echo-service" +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/EchoThriftClientModule3.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/EchoThriftClientModule3.scala new file mode 100644 index 0000000000..d942723c34 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/EchoThriftClientModule3.scala @@ -0,0 +1,10 @@ +package com.twitter.inject.thrift.integration.basic + +import com.twitter.inject.thrift.modules.ThriftClientModule +import com.twitter.test.thriftscala.EchoService + +object EchoThriftClientModule3 + extends ThriftClientModule[EchoService.MethodPerEndpoint] { + override val label = "echo-service" + override val dest = "flag!thrift-echo-service" +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/MyEchoService.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/MyEchoService.scala new file mode 100644 index 0000000000..aa3ec40518 --- /dev/null +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/MyEchoService.scala @@ -0,0 +1,31 @@ +package com.twitter.inject.thrift.integration.basic + +import com.twitter.finagle.thrift.ClientId +import com.twitter.inject.Logging +import com.twitter.test.thriftscala.EchoService +import com.twitter.util.Future +import java.util.concurrent.atomic.AtomicInteger +import javax.inject.Singleton + +@Singleton +class MyEchoService extends EchoService[Future] with Logging { + private val timesToEcho = new AtomicInteger(1) + + /* Public */ + override def echo(msg: String): Future[String] = { + info("echo " + msg) + assertClientId("echo-http-service") + Future.value(msg * timesToEcho.get) + } + override def setTimesToEcho(times: Int): Future[Int] = { + info("setTimesToEcho " + times) + assertClientId("echo-http-service") + timesToEcho.set(times) //mutation + Future(times) + } + + /* Private */ + private def assertClientId(name: String): Unit = { + assert(ClientId.current.contains(ClientId(name)), "Invalid Client ID: " + ClientId.current) + } +} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBHttpServer.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBHttpServer.scala index 4a8d9369db..6d708ede3c 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBHttpServer.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBHttpServer.scala @@ -8,7 +8,7 @@ import com.twitter.inject.thrift.modules.ThriftClientIdModule class ServiceBHttpServer extends HttpServer { override val modules: Seq[Module] = - Seq(ThriftClientIdModule, ServiceBServicePerEndpointModule) + Seq(ThriftClientIdModule, ServiceBThriftMethodBuilderClientModule) override def configureHttp(router: HttpRouter) { router diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBServicePerEndpointModule.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBThriftMethodBuilderClientModule.scala similarity index 81% rename from inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBServicePerEndpointModule.scala rename to inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBThriftMethodBuilderClientModule.scala index 5568c26a4c..d93f59f47d 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBServicePerEndpointModule.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBThriftMethodBuilderClientModule.scala @@ -3,12 +3,12 @@ package com.twitter.inject.thrift.integration.inheritance import com.google.inject.Module import com.twitter.inject.thrift.ThriftMethodBuilderFactory import com.twitter.inject.thrift.integration.filters.MethodLoggingTypeAgnosticFilter -import com.twitter.inject.thrift.modules.{ServicePerEndpointModule, ThriftClientIdModule} +import com.twitter.inject.thrift.modules.{PossiblyRetryableExceptions, ThriftClientIdModule, ThriftMethodBuilderClientModule} import com.twitter.serviceA.thriftscala.ServiceA import com.twitter.serviceB.thriftscala.ServiceB -object ServiceBServicePerEndpointModule - extends ServicePerEndpointModule[ServiceB.ServicePerEndpoint, ServiceB.MethodPerEndpoint] { +object ServiceBThriftMethodBuilderClientModule + extends ThriftMethodBuilderClientModule[ServiceB.ServicePerEndpoint, ServiceB.MethodPerEndpoint] { override val modules: Seq[Module] = Seq(ThriftClientIdModule) diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepServicePerEndpointModule.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepThriftMethodBuilderClientModule.scala similarity index 90% rename from inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepServicePerEndpointModule.scala rename to inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepThriftMethodBuilderClientModule.scala index 0b982c0a31..d5fa0c518c 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepServicePerEndpointModule.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepThriftMethodBuilderClientModule.scala @@ -6,15 +6,15 @@ import com.twitter.finagle.service.{ReqRep, ResponseClass, ResponseClassifier} import com.twitter.greeter.thriftscala.{Greeter, InvalidOperation} import com.twitter.inject.thrift.ThriftMethodBuilderFactory import com.twitter.inject.thrift.integration.filters.{HiLoggingTypeAgnosticFilter, MethodLoggingTypeAgnosticFilter} -import com.twitter.inject.thrift.modules.{ServicePerEndpointModule, ThriftClientIdModule} +import com.twitter.inject.thrift.modules.{PossiblyRetryableExceptions, ThriftMethodBuilderClientModule, ThriftClientIdModule} import com.twitter.util.tunable.Tunable import com.twitter.util.{Duration, Return, Throw} import scala.util.control.NonFatal -class GreeterReqRepServicePerEndpointModule( +class GreeterReqRepThriftMethodBuilderClientModule( requestHeaderKey: String, timeoutPerRequestTunable: Tunable[Duration] -) extends ServicePerEndpointModule[Greeter.ReqRepServicePerEndpoint, Greeter.MethodPerEndpoint] { +) extends ThriftMethodBuilderClientModule[Greeter.ReqRepServicePerEndpoint, Greeter.MethodPerEndpoint] { override val modules: Seq[Module] = Seq(ThriftClientIdModule) diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoServicePerEndpointModule.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoThriftMethodBuilderClientModule.scala similarity index 83% rename from inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoServicePerEndpointModule.scala rename to inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoThriftMethodBuilderClientModule.scala index 051d7badd3..7e97eb9fd4 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoServicePerEndpointModule.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoThriftMethodBuilderClientModule.scala @@ -3,11 +3,11 @@ package com.twitter.inject.thrift.integration.serviceperendpoint import com.google.inject.Module import com.twitter.inject.thrift.ThriftMethodBuilderFactory import com.twitter.inject.thrift.integration.filters.{MethodLoggingTypeAgnosticFilter, SetTimesEchoTypeAgnosticFilter} -import com.twitter.inject.thrift.modules.{ServicePerEndpointModule, ThriftClientIdModule} +import com.twitter.inject.thrift.modules.{PossiblyRetryableExceptions, ThriftClientIdModule, ThriftMethodBuilderClientModule} import com.twitter.test.thriftscala.EchoService -object EchoServicePerEndpointModule - extends ServicePerEndpointModule[EchoService.ServicePerEndpoint, EchoService.MethodPerEndpoint] { +object EchoThriftMethodBuilderClientModule + extends ThriftMethodBuilderClientModule[EchoService.ServicePerEndpoint, EchoService.MethodPerEndpoint] { override val modules: Seq[Module] = Seq(ThriftClientIdModule) diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterServicePerEndpointModule.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterThriftMethodBuilderClientModule.scala similarity index 89% rename from inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterServicePerEndpointModule.scala rename to inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterThriftMethodBuilderClientModule.scala index c7679f8a60..79a11dfa45 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterServicePerEndpointModule.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterThriftMethodBuilderClientModule.scala @@ -6,12 +6,12 @@ import com.twitter.greeter.thriftscala.Greeter.Bye import com.twitter.greeter.thriftscala.{Greeter, InvalidOperation} import com.twitter.inject.thrift.ThriftMethodBuilderFactory import com.twitter.inject.thrift.integration.filters.{HiLoggingTypeAgnosticFilter, MethodLoggingTypeAgnosticFilter} -import com.twitter.inject.thrift.modules.{ServicePerEndpointModule, ThriftClientIdModule} +import com.twitter.inject.thrift.modules.{PossiblyRetryableExceptions, ThriftClientIdModule, ThriftMethodBuilderClientModule} import com.twitter.util.{Return, Throw} import scala.util.control.NonFatal -object GreeterServicePerEndpointModule - extends ServicePerEndpointModule[Greeter.ServicePerEndpoint, Greeter.MethodPerEndpoint] { +object GreeterThriftMethodBuilderClientModule + extends ThriftMethodBuilderClientModule[Greeter.ServicePerEndpoint, Greeter.MethodPerEndpoint] { override val modules: Seq[Module] = Seq(ThriftClientIdModule) From 65561afe9d6695c9e0d0b06bc59efca1ed4d68c8 Mon Sep 17 00:00:00 2001 From: Chris Thalinger Date: Thu, 1 Feb 2018 00:49:45 +0000 Subject: [PATCH 08/12] finatra: fix build errors with JDK 9 Summary: Problem / Solution Minor syntax changes to support compilation with JDK 9. Signed-off-by: Daniel Schobel JIRA Issues: CSL-5935 Differential Revision: https://phabricator.twitter.biz/D133333 --- .../finatra/json/internal/streaming/ByteBufferUtils.scala | 2 +- .../finatra/json/internal/streaming/JsonArrayChunker.scala | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/jackson/src/main/scala/com/twitter/finatra/json/internal/streaming/ByteBufferUtils.scala b/jackson/src/main/scala/com/twitter/finatra/json/internal/streaming/ByteBufferUtils.scala index 59ccaffdf4..de61053780 100644 --- a/jackson/src/main/scala/com/twitter/finatra/json/internal/streaming/ByteBufferUtils.scala +++ b/jackson/src/main/scala/com/twitter/finatra/json/internal/streaming/ByteBufferUtils.scala @@ -36,7 +36,7 @@ private[finatra] object ByteBufferUtils extends Logging { val buf = Buf.ByteBuffer.Shared(copy) val str = buf.utf8str - debug(s"byteBuffer: $str pos: ${byteBuffer.position}") + debug(s"byteBuffer: $str pos: ${byteBuffer.position()}") } } } diff --git a/jackson/src/main/scala/com/twitter/finatra/json/internal/streaming/JsonArrayChunker.scala b/jackson/src/main/scala/com/twitter/finatra/json/internal/streaming/JsonArrayChunker.scala index 35f3deddbb..4ad4526156 100644 --- a/jackson/src/main/scala/com/twitter/finatra/json/internal/streaming/JsonArrayChunker.scala +++ b/jackson/src/main/scala/com/twitter/finatra/json/internal/streaming/JsonArrayChunker.scala @@ -72,7 +72,7 @@ private[finatra] class JsonArrayChunker extends Logging { parsingState = InsideString } // If the double quote wasn't escaped then this is the end of a string. - else if (in.get(in.position - 2) != '\\') { + else if (in.get(in.position() - 2) != '\\') { debug("State = Parsing") parsingState = Normal } @@ -83,7 +83,7 @@ private[finatra] class JsonArrayChunker extends Logging { private def extractBuf(): Buf = { val copy = byteBuffer.duplicate() copy.position(0) - copy.limit(byteBuffer.position - 1) + copy.limit(byteBuffer.position() - 1) val copyBuf = Buf.ByteBuffer.Shared(copy) byteBuffer = byteBuffer.slice() From fc271d1d74afbc599982f617b0b7a957cd217a5c Mon Sep 17 00:00:00 2001 From: Christopher Coco Date: Mon, 5 Feb 2018 17:19:36 +0000 Subject: [PATCH 09/12] finatra: Update building section of READMEs Summary: Problem/Solution Some documentation is stale after moving finatra/example projects to always part of the root project (even for released versions). Update the documentation to be clearer. TBR=true Differential Revision: https://phabricator.twitter.biz/D134491 --- CONTRIBUTING.md | 8 +------- examples/README.md | 10 ++++++++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ed2aef6c15..3e2de6d9c2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -27,13 +27,7 @@ locally if you are in master or a release branch. Finatra is built using [sbt][sbt]. When building please use the included [`./sbt`](https://github.com/twitter/finatra/blob/develop/sbt) script which provides a thin wrapper over [sbt][sbt] and correctly sets memory and other -settings. - -This is true for building all of Finatra *except* when in [master][master-branch] -(or any release tag) and building the `finatra/examples`. In [master][master-branch] -(and all release tags), the examples are defined such that they are able to be -built with your locally installed [sbt][sbt] since they are defined with their own -`build.sbt` files and use released Twitter OSS dependencies. +settings. If you have any questions or run into any problems, please create an issue here, tweet at us [@finatra](https://twitter.com/finatra), or email the diff --git a/examples/README.md b/examples/README.md index fe74446eb1..b3bb102034 100644 --- a/examples/README.md +++ b/examples/README.md @@ -12,12 +12,15 @@ at least one example which also uses [Maven](http://maven.apache.org). ## Building and Running -### `develop` branch +### To Build If you want to build/run the examples from the `develop` branch you will need to make sure to follow the instructions in the [CONTRIBUTING.md](../CONTRIBUTING.md) documentation for building SNAPSHOT versions of Finatra Twitter OSS dependencies along with building Finatra itself. +This is because in the `develop` branch the examples depend on `SNAPSHOT` versions +of the Twitter OSS libraries which you will need to build locally. + To accomplish this easily: ``` @@ -28,7 +31,10 @@ This commands differs from the [CONTRIBUTING.md](../CONTRIBUTING.md) documentati that it will also *include* building and publishing Finatra locally from the Finatra `develop` branch. -### `master` or a release branch +In the `master` branch, you should be able run the examples without building any dependencies +as `master` uses the released versions of the Twitter OSS dependencies. + +### To Run Follow the instructions in the project's `README.md` for how to run the server. ## More information From 582d46bc3d79a12789c208974ab197c02748615b Mon Sep 17 00:00:00 2001 From: Christopher Coco Date: Mon, 5 Feb 2018 18:24:13 +0000 Subject: [PATCH 10/12] inject-thrift: Add PossiblyRetryable.ResponseClassifier Summary: Problem/Solution Light clean up to have `c.t.inject.thrift.PossiblyRetryable` in inject-thrift express a ResponseClassifier instead of creating one separately in inject-thrift-client. TBR=true Differential Revision: https://phabricator.twitter.biz/D134328 --- CHANGELOG.md | 3 ++ .../inject/thrift/modules/package.scala | 19 ---------- .../basic/EchoThriftClientModule1.scala | 5 ++- ...viceBThriftMethodBuilderClientModule.scala | 5 ++- ...eqRepThriftMethodBuilderClientModule.scala | 7 ++-- .../EchoThriftMethodBuilderClientModule.scala | 7 ++-- ...eeterThriftMethodBuilderClientModule.scala | 7 ++-- .../inject/exceptions/PossiblyRetryable.scala | 38 ++++++++++++------- .../exceptions/PossiblyRetryableTest.scala | 1 + 9 files changed, 46 insertions(+), 46 deletions(-) delete mode 100644 inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/package.scala diff --git a/CHANGELOG.md b/CHANGELOG.md index 348283283c..09afea76c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,9 @@ All notable changes to this project will be documented in this file. Note that ` ### Changed +* inject-thrift: Update `c.t.inject.thrift.PossibleRetryable` to specify a ResponseClassifier + and update usages in inject-thrift-client to use it. ``PHAB_ID=D134328`` + * inject-thrift-client: Un-deprecate `c.t.inject.thrift.modules.ThriftClientModule` and update for parity with `ServicePerEndpointModule` in regards to ThriftMux client configuration. Update documentation. Rename `ServicePerEndpointModule` to diff --git a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/package.scala b/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/package.scala deleted file mode 100644 index 02e3ba6a34..0000000000 --- a/inject/inject-thrift-client/src/main/scala/com/twitter/inject/thrift/modules/package.scala +++ /dev/null @@ -1,19 +0,0 @@ -package com.twitter.inject.thrift - -import com.twitter.finagle.service.{ReqRep, ResponseClass, ResponseClassifier} -import com.twitter.inject.exceptions.PossiblyRetryable -import com.twitter.util.{Return, Throw} - -package object modules { - - val PossiblyRetryableExceptions: ResponseClassifier = - ResponseClassifier.named("PossiblyRetryableExceptions") { - case ReqRep(_, Throw(t)) if PossiblyRetryable.possiblyRetryable(t) => - ResponseClass.RetryableFailure - case ReqRep(_, Throw(_)) => - ResponseClass.NonRetryableFailure - case ReqRep(_, Return(_)) => - ResponseClass.Success - } - -} diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/EchoThriftClientModule1.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/EchoThriftClientModule1.scala index 9e65705a29..fd0e65d3d5 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/EchoThriftClientModule1.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/basic/EchoThriftClientModule1.scala @@ -1,7 +1,8 @@ package com.twitter.inject.thrift.integration.basic import com.twitter.finagle.ThriftMux -import com.twitter.inject.thrift.modules.{PossiblyRetryableExceptions, ThriftClientModule} +import com.twitter.inject.exceptions.PossiblyRetryable +import com.twitter.inject.thrift.modules.ThriftClientModule import com.twitter.test.thriftscala.EchoService import com.twitter.util.Future @@ -13,6 +14,6 @@ object EchoThriftClientModule1 extends ThriftClientModule[EchoService[Future]] { client: ThriftMux.Client ): ThriftMux.Client = { client - .withResponseClassifier(PossiblyRetryableExceptions) + .withResponseClassifier(PossiblyRetryable.ResponseClassifier) } } diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBThriftMethodBuilderClientModule.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBThriftMethodBuilderClientModule.scala index d93f59f47d..07288e8d01 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBThriftMethodBuilderClientModule.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/inheritance/ServiceBThriftMethodBuilderClientModule.scala @@ -1,9 +1,10 @@ package com.twitter.inject.thrift.integration.inheritance import com.google.inject.Module +import com.twitter.inject.exceptions.PossiblyRetryable import com.twitter.inject.thrift.ThriftMethodBuilderFactory import com.twitter.inject.thrift.integration.filters.MethodLoggingTypeAgnosticFilter -import com.twitter.inject.thrift.modules.{PossiblyRetryableExceptions, ThriftClientIdModule, ThriftMethodBuilderClientModule} +import com.twitter.inject.thrift.modules.{ThriftClientIdModule, ThriftMethodBuilderClientModule} import com.twitter.serviceA.thriftscala.ServiceA import com.twitter.serviceB.thriftscala.ServiceB @@ -22,7 +23,7 @@ object ServiceBThriftMethodBuilderClientModule servicePerEndpoint .withEcho( builder.method[ServiceA.Echo.Args, ServiceA.Echo.SuccessType](ServiceA.Echo) - .withRetryForClassifier(PossiblyRetryableExceptions) + .withRetryForClassifier(PossiblyRetryable.ResponseClassifier) .withAgnosticFilter(new MethodLoggingTypeAgnosticFilter()) .filtered[EchoFilter] .service) diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepThriftMethodBuilderClientModule.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepThriftMethodBuilderClientModule.scala index d5fa0c518c..86a3265429 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepThriftMethodBuilderClientModule.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/reqrepserviceperendpoint/GreeterReqRepThriftMethodBuilderClientModule.scala @@ -4,9 +4,10 @@ import com.google.inject.Module import com.twitter.conversions.percent._ import com.twitter.finagle.service.{ReqRep, ResponseClass, ResponseClassifier} import com.twitter.greeter.thriftscala.{Greeter, InvalidOperation} +import com.twitter.inject.exceptions.PossiblyRetryable import com.twitter.inject.thrift.ThriftMethodBuilderFactory import com.twitter.inject.thrift.integration.filters.{HiLoggingTypeAgnosticFilter, MethodLoggingTypeAgnosticFilter} -import com.twitter.inject.thrift.modules.{PossiblyRetryableExceptions, ThriftMethodBuilderClientModule, ThriftClientIdModule} +import com.twitter.inject.thrift.modules.{ThriftClientIdModule, ThriftMethodBuilderClientModule} import com.twitter.util.tunable.Tunable import com.twitter.util.{Duration, Return, Throw} import scala.util.control.NonFatal @@ -34,7 +35,7 @@ class GreeterReqRepThriftMethodBuilderClientModule( .withAgnosticFilter(new HiLoggingTypeAgnosticFilter) // method type-specific filter .filtered(new HiHeadersFilter(requestHeaderKey)) - .withRetryForClassifier(PossiblyRetryableExceptions) + .withRetryForClassifier(PossiblyRetryable.ResponseClassifier) .idempotent(1.percent) .service ) @@ -55,7 +56,7 @@ class GreeterReqRepThriftMethodBuilderClientModule( .filtered(new ByeHeadersFilter(requestHeaderKey)) // method type-specific filter .filtered[ByeFilter] - .withRetryForClassifier(PossiblyRetryableExceptions) + .withRetryForClassifier(PossiblyRetryable.ResponseClassifier) .service ) // global filter diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoThriftMethodBuilderClientModule.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoThriftMethodBuilderClientModule.scala index 7e97eb9fd4..5f8d5a98ff 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoThriftMethodBuilderClientModule.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/EchoThriftMethodBuilderClientModule.scala @@ -1,9 +1,10 @@ package com.twitter.inject.thrift.integration.serviceperendpoint import com.google.inject.Module +import com.twitter.inject.exceptions.PossiblyRetryable import com.twitter.inject.thrift.ThriftMethodBuilderFactory import com.twitter.inject.thrift.integration.filters.{MethodLoggingTypeAgnosticFilter, SetTimesEchoTypeAgnosticFilter} -import com.twitter.inject.thrift.modules.{PossiblyRetryableExceptions, ThriftClientIdModule, ThriftMethodBuilderClientModule} +import com.twitter.inject.thrift.modules.{ThriftClientIdModule, ThriftMethodBuilderClientModule} import com.twitter.test.thriftscala.EchoService object EchoThriftMethodBuilderClientModule @@ -24,13 +25,13 @@ object EchoThriftMethodBuilderClientModule builder.method[EchoService.Echo.Args, EchoService.Echo.SuccessType](EchoService.Echo) // method type-specific filter .filtered[EchoFilter] - .withRetryForClassifier(PossiblyRetryableExceptions) + .withRetryForClassifier(PossiblyRetryable.ResponseClassifier) .service) .withSetTimesToEcho( builder.method(EchoService.SetTimesToEcho) // method type-agnostic filter .withAgnosticFilter(new SetTimesEchoTypeAgnosticFilter()) - .withRetryForClassifier(PossiblyRetryableExceptions) + .withRetryForClassifier(PossiblyRetryable.ResponseClassifier) .service) // global (type-agnostic) filter .filtered(new MethodLoggingTypeAgnosticFilter()) diff --git a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterThriftMethodBuilderClientModule.scala b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterThriftMethodBuilderClientModule.scala index 79a11dfa45..7fb2489672 100644 --- a/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterThriftMethodBuilderClientModule.scala +++ b/inject/inject-thrift-client/src/test/scala/com/twitter/inject/thrift/integration/serviceperendpoint/GreeterThriftMethodBuilderClientModule.scala @@ -4,9 +4,10 @@ import com.google.inject.Module import com.twitter.finagle.service.{ReqRep, ResponseClass, ResponseClassifier} import com.twitter.greeter.thriftscala.Greeter.Bye import com.twitter.greeter.thriftscala.{Greeter, InvalidOperation} +import com.twitter.inject.exceptions.PossiblyRetryable import com.twitter.inject.thrift.ThriftMethodBuilderFactory import com.twitter.inject.thrift.integration.filters.{HiLoggingTypeAgnosticFilter, MethodLoggingTypeAgnosticFilter} -import com.twitter.inject.thrift.modules.{PossiblyRetryableExceptions, ThriftClientIdModule, ThriftMethodBuilderClientModule} +import com.twitter.inject.thrift.modules.{ThriftClientIdModule, ThriftMethodBuilderClientModule} import com.twitter.util.{Return, Throw} import scala.util.control.NonFatal @@ -28,7 +29,7 @@ object GreeterThriftMethodBuilderClientModule builder.method[Greeter.Hi.Args, Greeter.Hi.SuccessType](Greeter.Hi) // method type-agnostic filter .withAgnosticFilter[HiLoggingTypeAgnosticFilter] - .withRetryForClassifier(PossiblyRetryableExceptions) + .withRetryForClassifier(PossiblyRetryable.ResponseClassifier) .service) .withHello( builder.method(Greeter.Hello) @@ -40,7 +41,7 @@ object GreeterThriftMethodBuilderClientModule builder.method[Bye.Args, Bye.SuccessType](Greeter.Bye) // method type-specific filter .filtered[ByeFilter] - .withRetryForClassifier(PossiblyRetryableExceptions) + .withRetryForClassifier(PossiblyRetryable.ResponseClassifier) .service) // global (type-agnostic) filter .filtered(new MethodLoggingTypeAgnosticFilter()) diff --git a/inject/inject-thrift/src/main/scala/com/twitter/inject/exceptions/PossiblyRetryable.scala b/inject/inject-thrift/src/main/scala/com/twitter/inject/exceptions/PossiblyRetryable.scala index cb9fd34606..f77161af17 100644 --- a/inject/inject-thrift/src/main/scala/com/twitter/inject/exceptions/PossiblyRetryable.scala +++ b/inject/inject-thrift/src/main/scala/com/twitter/inject/exceptions/PossiblyRetryable.scala @@ -1,12 +1,8 @@ package com.twitter.inject.exceptions import com.twitter.finagle.mux.ClientDiscardedRequestException -import com.twitter.finagle.{ - BackupRequestLost, - CancelledConnectionException, - CancelledRequestException, - Failure -} +import com.twitter.finagle.{BackupRequestLost, CancelledConnectionException, CancelledRequestException, Failure, service => ctfs} +import com.twitter.finagle.service.{ReqRep, ResponseClass} import com.twitter.util.{Return, Throw, Try} import scala.util.control.NonFatal @@ -21,23 +17,37 @@ import scala.util.control.NonFatal */ object PossiblyRetryable { + /** + * Partial function which can be used to determine if a request is possibly + * retryable. + */ val PossiblyRetryableExceptions: PartialFunction[Try[_], Boolean] = { case Return(_) => false case Throw(t) => possiblyRetryable(t) } + /** + * A [[com.twitter.finagle.service.ResponseClassifier]] which uses the + * [[PossiblyRetryableExceptions]] partial function to classify responses as + * retryable. + */ + val ResponseClassifier: ctfs.ResponseClassifier = + ctfs.ResponseClassifier.named("PossiblyRetryableExceptions") { + case ReqRep(_, Throw(t)) if PossiblyRetryable.possiblyRetryable(t) => + ResponseClass.RetryableFailure + case ReqRep(_, Throw(_)) => + ResponseClass.NonRetryableFailure + case ReqRep(_, Return(_)) => + ResponseClass.Success + } + def apply(t: Throwable): Boolean = { PossiblyRetryable.possiblyRetryable(t) } - def unapply(t: Throwable): Option[Throwable] = { - if (apply(t)) - Some(t) - else - None - } + def unapply(t: Throwable): Option[Throwable] = Some(t).filter(apply) - // TODO: RetryableWriteException's are automatically retried by Finagle (how many times?), so we should avoid retrying again here + // TODO: RetryableWriteException's are automatically retried by Finagle, so we should avoid retrying again here def possiblyRetryable(t: Throwable): Boolean = { !isCancellation(t) && !t.isInstanceOf[NonRetryableException] && @@ -49,7 +59,7 @@ object PossiblyRetryable { case _: CancelledRequestException => true case _: CancelledConnectionException => true case _: ClientDiscardedRequestException => true - case f: Failure if f.isFlagged(Failure.Interrupted) => true + case f: Failure if f.isFlagged(Failure.Interrupted) || f.isFlagged(Failure.Ignorable) => true case f: Failure if f.cause.isDefined => isCancellation(f.cause.get) case _ => false } diff --git a/inject/inject-thrift/src/test/scala/com/twitter/inject/tests/exceptions/PossiblyRetryableTest.scala b/inject/inject-thrift/src/test/scala/com/twitter/inject/tests/exceptions/PossiblyRetryableTest.scala index a038859ed6..ad39720227 100644 --- a/inject/inject-thrift/src/test/scala/com/twitter/inject/tests/exceptions/PossiblyRetryableTest.scala +++ b/inject/inject-thrift/src/test/scala/com/twitter/inject/tests/exceptions/PossiblyRetryableTest.scala @@ -24,6 +24,7 @@ class PossiblyRetryableTest extends Test { assertIsCancellation(new CancelledRequestException) assertIsCancellation(new CancelledConnectionException(new Exception("cause"))) assertIsCancellation(ClientDiscardedRequestException("cause")) + assertIsCancellation(Failure("int", Failure.Ignorable)) assertIsCancellation(Failure("int", Failure.Interrupted)) assertIsCancellation(Failure.rejected("", new CancelledRequestException)) } From 8a8aee484aa4651dde8450af2f1253da83a7e4b4 Mon Sep 17 00:00:00 2001 From: Christopher Coco Date: Mon, 5 Feb 2018 20:47:12 +0000 Subject: [PATCH 11/12] inject-thrift: More clean-up of c.t.inject.thrift.PossiblyRetryable Summary: Problem/Solution Update `c.t.inject.thrift.PossiblyRetryable` Differential Revision: https://phabricator.twitter.biz/D134541 --- .../inject/exceptions/PossiblyRetryable.scala | 33 ++++++++++++++----- .../exceptions/PossiblyRetryableTest.scala | 10 +++++- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/inject/inject-thrift/src/main/scala/com/twitter/inject/exceptions/PossiblyRetryable.scala b/inject/inject-thrift/src/main/scala/com/twitter/inject/exceptions/PossiblyRetryable.scala index f77161af17..78f66b36f8 100644 --- a/inject/inject-thrift/src/main/scala/com/twitter/inject/exceptions/PossiblyRetryable.scala +++ b/inject/inject-thrift/src/main/scala/com/twitter/inject/exceptions/PossiblyRetryable.scala @@ -19,7 +19,9 @@ object PossiblyRetryable { /** * Partial function which can be used to determine if a request is possibly - * retryable. + * retryable. "Possibly" retryable means the returned exception is '''not''' + * a cancellation, does '''not''' represent an exception of failure that should + * not be tried and lastly is non-fatal. */ val PossiblyRetryableExceptions: PartialFunction[Try[_], Boolean] = { case Return(_) => false @@ -45,22 +47,37 @@ object PossiblyRetryable { PossiblyRetryable.possiblyRetryable(t) } - def unapply(t: Throwable): Option[Throwable] = Some(t).filter(apply) + def unapply(t: Throwable): Option[Throwable] = + if (apply(t)) Some(t) else None - // TODO: RetryableWriteException's are automatically retried by Finagle, so we should avoid retrying again here + /** + * A [[Throwable]] is "possibly" retryable if: + * a. it is not a cancellation + * b. it is not an exception or failure which should not be retried + * c. is a non-fatal exception + * @param t the [[Throwable]] to inspect + * @return true if the [[Throwable]] represents an Exception or Failure which is possibly + * retryable, false otherwise. + */ def possiblyRetryable(t: Throwable): Boolean = { - !isCancellation(t) && - !t.isInstanceOf[NonRetryableException] && - NonFatal(t) + !isCancellation(t) && !isNonRetryable(t) && NonFatal(t) } - def isCancellation(t: Throwable): Boolean = t match { + private[inject] def isCancellation(t: Throwable): Boolean = t match { case BackupRequestLost => true case _: CancelledRequestException => true case _: CancelledConnectionException => true case _: ClientDiscardedRequestException => true - case f: Failure if f.isFlagged(Failure.Interrupted) || f.isFlagged(Failure.Ignorable) => true + case f: Failure if f.isFlagged(Failure.Interrupted) => true case f: Failure if f.cause.isDefined => isCancellation(f.cause.get) case _ => false } + + private[inject] def isNonRetryable(t: Throwable) : Boolean = t match { + case BackupRequestLost => true + case _: NonRetryableException => true + case f: Failure if f.isFlagged(Failure.Ignorable) => true + case f: Failure if f.cause.isDefined => isNonRetryable(f.cause.get) + case _ => false + } } diff --git a/inject/inject-thrift/src/test/scala/com/twitter/inject/tests/exceptions/PossiblyRetryableTest.scala b/inject/inject-thrift/src/test/scala/com/twitter/inject/tests/exceptions/PossiblyRetryableTest.scala index ad39720227..2c7b387625 100644 --- a/inject/inject-thrift/src/test/scala/com/twitter/inject/tests/exceptions/PossiblyRetryableTest.scala +++ b/inject/inject-thrift/src/test/scala/com/twitter/inject/tests/exceptions/PossiblyRetryableTest.scala @@ -24,11 +24,15 @@ class PossiblyRetryableTest extends Test { assertIsCancellation(new CancelledRequestException) assertIsCancellation(new CancelledConnectionException(new Exception("cause"))) assertIsCancellation(ClientDiscardedRequestException("cause")) - assertIsCancellation(Failure("int", Failure.Ignorable)) assertIsCancellation(Failure("int", Failure.Interrupted)) assertIsCancellation(Failure.rejected("", new CancelledRequestException)) } + test("test isNonRetryable") { + assertIsNonRetryable(BackupRequestLost) + assertIsNonRetryable(Failure("int", Failure.Ignorable)) + } + test("test apply") { PossiblyRetryable(PossiblyRetryableException) should be(true) PossiblyRetryable(NonRetryableException) should be(false) @@ -72,4 +76,8 @@ class PossiblyRetryableTest extends Test { private def assertIsCancellation(e: Throwable) { PossiblyRetryable.isCancellation(e) should be(true) } + + private def assertIsNonRetryable(e: Throwable) { + PossiblyRetryable.isNonRetryable(e) should be(true) + } } From 16da53321038149947974e569a50b48a91b6aff1 Mon Sep 17 00:00:00 2001 From: Yufan Gong Date: Mon, 5 Feb 2018 23:17:46 +0000 Subject: [PATCH 12/12] Twitter-oss: Prepare OSS libraries for release 18.2.0 Summary: Problem We want to release the next versions of our Twitter OSS libraries 18.2.0 - util - scrooge - finagle - twitter-server - finatra Solution Prepare libraries for their next releases. JIRA Issues: CSL-5938 Differential Revision: https://phabricator.twitter.biz/D134680 --- CHANGELOG.md | 10 ++++++++++ README.md | 4 ++-- build.sbt | 2 +- project/plugins.sbt | 2 +- 4 files changed, 14 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 09afea76c2..91d1b1bdd5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,16 @@ All notable changes to this project will be documented in this file. Note that ` ### Added +### Changed + +### Fixed + +### Closed + +## [finatra-18.2.0](https://github.com/twitter/finatra/tree/finatra-18.2.0) (2018-02-05) + +### Added + * inject-thrift-client: Add methods to `c.t.inject.thrift.filters.ThriftClientFilterChain` to allow Tunable timeouts and request timeouts. ``PHAB_ID=D128506`` diff --git a/README.md b/README.md index ff0a90b45f..e52b4c0e05 100644 --- a/README.md +++ b/README.md @@ -1,7 +1,7 @@ # Finatra -[![Build Status](https://secure.travis-ci.org/twitter/finatra.png?branch=develop)](https://travis-ci.org/twitter/finatra?branch=develop) -[![Test Coverage](https://codecov.io/github/twitter/finatra/coverage.svg?branch=develop)](https://codecov.io/github/twitter/finatra?branch=develop) +[![Build Status](https://secure.travis-ci.org/twitter/finatra.png?branch=master)](https://travis-ci.org/twitter/finatra?branch=master) +[![Test Coverage](https://codecov.io/github/twitter/finatra/coverage.svg?branch=master)](https://codecov.io/github/twitter/finatra?branch=master) [![Project status](https://img.shields.io/badge/status-active-brightgreen.svg)](#status) [![Maven Central](https://maven-badges.herokuapp.com/maven-central/com.twitter/finatra-http_2.12/badge.svg)][maven-central] [![Gitter](https://badges.gitter.im/Join%20Chat.svg)](https://gitter.im/twitter/finatra) diff --git a/build.sbt b/build.sbt index 4547ff8e21..1321458b88 100644 --- a/build.sbt +++ b/build.sbt @@ -4,7 +4,7 @@ import scoverage.ScoverageKeys concurrentRestrictions in Global += Tags.limit(Tags.Test, 1) // All Twitter library releases are date versioned as YY.MM.patch -val releaseVersion = "18.2.0-SNAPSHOT" +val releaseVersion = "18.2.0" lazy val buildSettings = Seq( version := releaseVersion, diff --git a/project/plugins.sbt b/project/plugins.sbt index f8ee3bbb00..2faaa09158 100644 --- a/project/plugins.sbt +++ b/project/plugins.sbt @@ -3,7 +3,7 @@ resolvers ++= Seq( Resolver.sonatypeRepo("snapshots") ) -val releaseVersion = "18.2.0-SNAPSHOT" +val releaseVersion = "18.2.0" addSbtPlugin("com.twitter" % "scrooge-sbt-plugin" % releaseVersion)