Merge commit '16da53321038149947974e569a50b48a91b6aff1'

CHANGELOG.md

@@ -12,6 +12,35 @@ All notable changes to this project will be documented in this file. Note that `
 
 ### 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``
+
+* 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``
+
+### 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
+  the more descriptive and consistently named `ThriftMethodBuilderClientModule`. 
+  ``PHAB_ID=D129891``
+
+### Fixed
+
+### Closed
+
 ## [finatra-18.1.0](https://github.com/twitter/finatra/tree/finatra-18.1.0) (2018-01-17)
 
 ### Added

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

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
 

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"
 
 lazy val buildSettings = Seq(
   version := releaseVersion,
@@ -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 := "<empty>;.*\\.thriftscala.*;.*\\.thriftjava.*",
+    ScoverageKeys.coverageExcludedPackages := "<empty>;.*\\.thriftscala.*;.*\\.thriftjava.*;.*LatencyFilter.*",
     libraryDependencies ++= Seq(
       "com.twitter" %% "finagle-exp" % versions.twLibVersion,
       "com.twitter" %% "finagle-thrift" % versions.twLibVersion,

doc/README.md

@@ -17,9 +17,9 @@ $ ./pushsite.bash
 <br/ >  
 <br/ >  
 
-#### 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
+[finagle]: https://github.com/twitter/finagle

doc/src/sphinx/user-guide/http/clients.rst

@@ -0,0 +1,6 @@
+.. _http_clients:
+
+Communicate with an HTTP Service
+================================
+
+*COMING SOON*

doc/src/sphinx/user-guide/http/server.rst

@@ -152,4 +152,9 @@ For example:
     }
 
 
-For more information on `Finagle <https://github.com/twitter/finagle>`__ server configuration see the documentation `here <https://twitter.github.io/finagle/guide/Configuration.html>`__; specifically the server documentation `here <https://twitter.github.io/finagle/guide/Servers.html>`__.
+For more information on `Finagle <https://github.com/twitter/finagle>`__ server configuration see the documentation `here <https://twitter.github.io/finagle/guide/Configuration.html>`__; specifically the server documentation `here <https://twitter.github.io/finagle/guide/Servers.html>`__.
+
+Testing
+-------
+
+For information on testing an HTTP server see the HTTP Server `Feature Tests <../testing/feature_tests.html#http-server>`__ section.

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

doc/src/sphinx/user-guide/testing/feature_tests.rst

@@ -3,14 +3,26 @@
 Feature Tests
 =============
 
-If you are familiar with `Gherkin <http://docs.behat.org/en/v2.5/guides/1.gherkin.html>`__ or `Cucumber <https://github.com/cucumber/cucumber/wiki/Feature-Introduction>`__ or other similar testing languages and frameworks, then `feature testing <https://wiki.documentfoundation.org/QA/Testing/Feature_Tests>`__ will feel somewhat familiar. In Finatra, a feature test always consists of an app or a server under test. See the `FeatureTest <https://github.com/twitter/finatra/blob/develop/inject/inject-server/src/test/scala/com/twitter/inject/server/FeatureTest.scala>`__ 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 <http://docs.behat.org/en/v2.5/guides/1.gherkin.html>`__ or
+`Cucumber <https://github.com/cucumber/cucumber/wiki/Feature-Introduction>`__ or other similar
+testing languages and frameworks, then `feature testing <https://wiki.documentfoundation.org/QA/Testing/Feature_Tests>`__
+will feel somewhat familiar. In Finatra, a feature test always consists of an app or a server under
+test. See the `FeatureTest <https://github.com/twitter/finatra/blob/develop/inject/inject-server/src/test/scala/com/twitter/inject/server/FeatureTest.scala>`__
+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 <https://twitter.github.io/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 <https://twitter.github.io/scrooge/Finagle.html>`__
+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,15 +153,18 @@ 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")
         }
       }
     }
 
 
 .. 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

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 <feature_tests.html>`__ 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 <https://github.com/twitter/finatra/blob/develop/inject/inject-app/src/test/scala/com/twitter/inject/app/TestInjector.scala>`__
 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 <https://twitter.github.io/finagle/guide/Clients.html>`__
 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 <feature_tests.html#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
+