Add twitter future support #128

ghost · 2017-08-03T15:31:39Z

As a finagle user I'd love to use fetch as a first class citizen in the whole twitter ecosystem. This PR adds the implicit for com.twitter.util.Future to do that. We also use @travisbrown catbird to provide us with the instance/syntax/implicits support for the twitter ecosystem so I've also added support for Rerunnable in this PR.

this is jvm only, so I'm not sure exactly how to make the work here but the core of this works. Twitter future and Catbirds Rerunnable are now supported

and execute the tests to verify they're passing

this matches the monix side

raulraja · 2017-08-03T21:30:25Z

twitter/jvm/src/main/scala/TwitterFuture.scala

+        case Async(ac, timeout) ⇒
+          Rerunnable.fromFuture {
+            val p: Promise[A] = Promise()
+            FuturePool.unboundedPool(ac(p setValue _, p setException _))


@chris-vale-ck is it common practice in Finagle to use the unbounded pool for these cases or should it be required as implicit in fetchRerunableMonadError to allow pools with different policies?

So yeah, I know how it sounds to non-finaglers but yes using the default unbounded pool is how things roll. That pool is the same pool that the rest of finagle uses. I can gen up a new pool with some limits on it if it's desired, or even leave the choice of FuturePool up in the air.

raulraja · 2017-08-03T21:31:32Z

twitter/jvm/src/main/scala/TwitterFuture.scala

+    case e            => Rerunnable.fromFuture(Future(e.value))
+  }
+
+  implicit def fetchRerunableMonadError: FetchMonadError[Rerunnable] =


fetchRerunableMonadError should probably be fetchRerunnableMonadError

raulraja · 2017-08-03T21:32:22Z

twitter/jvm/src/main/scala/TwitterFuture.scala

+      }
+    }
+
+  implicit def fetchTwFutureMonadError: FetchMonadError[Future] =


https://github.com/47deg/fetch/pull/128/files#r131264794 also applies here

tomjadams

We also make use of Twitter Futures so this is a very welcome contribution.

Here is a gist I shared in the Fetch channel several months ago, which might be worth considering pulling some things from, specifically timeouts & parameterising the future pool.

https://gist.github.com/tomjadams/2ca1111532ee7ce0f550d5d0bd9771e5

tomjadams · 2017-08-03T21:33:48Z

twitter/jvm/src/main/scala/TwitterFuture.scala

+        case Async(ac, timeout) ⇒
+          Rerunnable.fromFuture {
+            val p: Promise[A] = Promise()
+            FuturePool.unboundedPool(ac(p setValue _, p setException _))


I'm uncomfortable about using an unbounded pool here, would rather see it passed in (implicitly), giving the user more control.

tomjadams · 2017-08-03T21:34:17Z

twitter/jvm/src/main/scala/TwitterFuture.scala

+        case Sync(e) ⇒ Future(e.value)
+        case Async(ac, timeout) ⇒
+          val p: Promise[A] = Promise()
+          FuturePool.unboundedPool(ac(p setValue _, p setException _))


As per the above comment, would prefer this used a passed pool rather than this one.

tomjadams · 2017-08-03T21:34:49Z

twitter/jvm/src/main/scala/TwitterFuture.scala

+    new FetchMonadError.FromMonadError[Future] {
+      override def runQuery[A](j: Query[A]): Future[A] = j match {
+        case Sync(e) ⇒ Future(e.value)
+        case Async(ac, timeout) ⇒


Would be good to see this taking the timeout into account. We don't use it specifically, but I guess some people might, so good to support that.

See also this pending PR regarding timeout capabilities #127

tomjadams · 2017-08-03T21:36:56Z

twitter/jvm/src/main/scala/TwitterFuture.scala

+  implicit def fetchTwFutureMonadError: FetchMonadError[Future] =
+    new FetchMonadError.FromMonadError[Future] {
+      override def runQuery[A](j: Query[A]): Future[A] = j match {
+        case Sync(e) ⇒ Future(e.value)


Are there semantic differences here between Future.apply and Future.value that make sense to use one vs. the other?

e.value could potentially throw an exception, so I think we need Future.apply here.

You could also use catchNonFatalEval (from cats.ApplicativeError) instead.

Just thinking about it. From a callers POV they just get a Future. But using apply would schedule that code to be run on some thread whereas value would just return a completed future without the need for any async evaluation.

@peterneyens Good point, hadn’t thought of that.

@tomjadams For Twitter's Future apply will evaluate the thunk in the calling thread, so I think it's fine to use here.

@travisbrown Yeah, I did read that after I posted the initial comment. But that'd be more work than simply returning a value though right?

@tomjadams Yeah, there's a little more work in that you have to instantiate an extra => A and Try, but dropping down to a try-catch block to avoid that feels like premature optimization (and there are probably a ton of other places with similar overhead).

with two n's in ht middle

codecov-io · 2017-08-03T22:27:28Z

Codecov Report

Merging #128 into master will increase coverage by 0.63%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##           master     #128      +/-   ##
==========================================
+ Coverage      78%   78.63%   +0.63%     
==========================================
  Files          14       15       +1     
  Lines         300      323      +23     
  Branches        2        2              
==========================================
+ Hits          234      254      +20     
- Misses         66       69       +3

Impacted Files	Coverage Δ
twitter/jvm/src/main/scala/TwitterFuture.scala	`82.35% <82.35%> (ø)`
shared/src/main/scala/implicits.scala	`100% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae93d41...17a8d6b. Read the comment docs.

with the default value of the interruptableUnboundedPool and then use the fact that the pool creates interruptable threads to wire up the timeouts.

ghost · 2017-08-03T22:45:32Z

I've gone ahead and added an implicit ThreadPool but with the default value of ThreadPool.interruptableUnboundedPool

rely on `catbird` to provide the twitter `util-core` dependency.

Twitter util no longer supports 2.10 so this won't build with it in. also remove the now unneeded version check for including paradise.

oops, missed travis

test evalToRunnable

for both Rerunnable and Future ... the code is there and it should work so prove it.

raulraja · 2017-08-07T01:49:32Z

Looks like codecov is not reporting properly since tests for that file are included. @dominv do you think you can take a look at codecov here in case it's missconfigured?

dominv · 2017-08-07T16:47:07Z

@raulraja Sure, I'll find out what's happening

for both twitter future and catbirds rerunnable

peterneyens

I think we are using => and <- everywhere, can you replace the unicode ⇒ and ← ?

peterneyens · 2017-08-15T09:18:51Z

twitter/jvm/src/main/scala/TwitterFuture.scala

+            }
+          }
+        case Ap(qf, qx) ⇒
+          runQuery(qf).product(Rerunnable(qx)) map { case (f, x) ⇒ f(x) }


Shouldn't this be runQuery(qf).product(runQuery(qx))?

without the map the statement produces a Rerunnable[(Any => A, Any)] so we need to run the (Any) => A on the Any. or at least that's what I think needs to be done.

Sorry for the confusion, I meant runQuery(qf).product(runQuery(qx)).map { case (f, x) => f(x) }.

So thanks 👍

oh, yeah you're right.

peterneyens · 2017-08-15T09:21:13Z

twitter/jvm/src/main/scala/TwitterFuture.scala

+            case _ => p
+          }
+        case Ap(qf, qx) ⇒
+          runQuery(qf).join(runQuery(qx)).map { case (f, x) ⇒ f(x) }


catbird overrides ap exactly in this way, so we could just write ap(runQuery(qf))(runQuery(qx))

missed that

to use the catbird provided ap

peterneyens · 2017-08-15T16:21:05Z

twitter/jvm/src/main/scala/TwitterFuture.scala

+  ): FetchMonadError[Rerunnable] =
+    new FetchMonadError.FromMonadError[Rerunnable] {
+      override def runQuery[A](j: Query[A]): Rerunnable[A] = j match {
+        case Sync(e) ⇒ Rerunnable { e.value }


Should we use evalToRerunnable here?

in the rerunnable sync case

peterneyens

Thanks @chris-vale-ck.

Thanks for reviewing @tomjadams and @travisbrown.

dominv · 2017-08-17T01:12:48Z

@raulraja For what I've seen so far, I believe that it is not possible to access to the files' individual reports from the Files tab https://codecov.io/gh/47deg/fetch/pull/128/tree because the branch that originated the PR belongs to a forked repository and it seems that codecov can't find it.

Nevertheless, it's possible to access to this reports using the Commits tab where you can find the same percentages than before https://codecov.io/gh/47deg/fetch/tree/48e67edc7d36960ec1d3ed1786178293f66e3041 but, now, it also allows to access to each file's report, including TwitterFuture.scala https://codecov.io/gh/47deg/fetch/src/48e67edc7d36960ec1d3ed1786178293f66e3041/twitter/jvm/src/main/scala/TwitterFuture.scala

peterneyens · 2017-08-17T08:51:18Z

twitter/jvm/src/main/scala/TwitterFuture.scala

+ * limitations under the License.
+ */
+
+package fetch.twitterFuture


It might be strange to call this "twitterFuture" while it also contains Rerunnable. Maybe just "twitter" or "twitterUtil"?

peterneyens · 2017-08-17T09:01:40Z

Thanks for looking into this @dominv.

I'll cover the untested lines later, unless you don't mind adding a test similar to the ones added in #127 to cover the timeouts @chris-vale-ck?

adapted from the scala future version

when testing timeouts

ghost · 2017-08-17T16:13:38Z

@peterneyens I went ahead and wrote the timeout tests for both twitter future and catbirds rerunnable versions. Well when I say wrote I mean adapted form the tests you mentioned.

travisbrown · 2017-08-17T17:15:14Z

Two small notes:

I've run into a stack-safety issue in catbird 0.16.0 that only turned up in the iteratee.io tests. Straightforward recursive monadic binding is fine (and its stack-safety is tested in catbird itself), and I'm trying to characterize the issue today. There is likely to be a catbird 0.17.0 release soon to fix this, though.
Finagle 7 is apparently just around the corner, which will also require a new catbird release. More generally, the fact that Twitter publishes breaking Util updates every month or two means that maintaining a library with a Twitter Util dependency can be a little annoying. (Now that Cats is approaching 1.0 I'm considering splitting iteratee-twitter out of the iteratee.io repo, for example.)

peterneyens · 2017-08-17T17:34:22Z

Thanks for the info @travisbrown.

The next Fetch release needs to wait for a Monix release for Cats 1.0.0-MF anyway, so waiting on 0.17.0 and maybe another update for Finagle 7 is not a problem.
We can always follow a similar strategy later on and create a finch-twitter repo if the Twitter Util dependency updates become a problem.

@chris-vale-ck Thanks for the tests.

Chris Vale added 3 commits August 2, 2017 16:20

initial cut at twitter support

8488d90

this is jvm only, so I'm not sure exactly how to make the work here but the core of this works. Twitter future and Catbirds Rerunnable are now supported

make the thing build

eec54a6

and execute the tests to verify they're passing

add an evalToRerunnable

4e46196

this matches the monix side

peterneyens self-requested a review August 3, 2017 15:41

raulraja reviewed Aug 3, 2017

View reviewed changes

tomjadams reviewed Aug 3, 2017

View reviewed changes

spell rerunnable the conventional way

0deb163

with two n's in ht middle

add implicit FuturePool

99ef02f

with the default value of the interruptableUnboundedPool and then use the fact that the pool creates interruptable threads to wire up the timeouts.

Chris Vale added 6 commits August 3, 2017 15:56

simplify dependencies

bd3bfff

rely on `catbird` to provide the twitter `util-core` dependency.

remove 2.10 cross version

c3693e2

Twitter util no longer supports 2.10 so this won't build with it in. also remove the now unneeded version check for including paradise.

remove 2.10

fe605ec

oops, missed travis

extend test coverage

99b0e69

test evalToRunnable

Merge branch 'master' of github.com:chris-vale-ck/fetch

dd6203a

test sync

05b6710

for both Rerunnable and Future ... the code is there and it should work so prove it.

add applicative usage tests

7de23e3

for both twitter future and catbirds rerunnable

peterneyens reviewed Aug 15, 2017

View reviewed changes

update twitter future Ap handler

1c3145e

to use the catbird provided ap

peterneyens reviewed Aug 15, 2017

View reviewed changes

use evalToRerunnable

48e67ed

in the rerunnable sync case

peterneyens approved these changes Aug 15, 2017

View reviewed changes

peterneyens reviewed Aug 17, 2017

View reviewed changes

add test to excercise twfuture fetch timeouts

fa43be0

adapted from the scala future version

also test Rerunnable side

17a8d6b

when testing timeouts

peterneyens merged commit 93a7d41 into xebia-functional:master Aug 17, 2017

Add twitter future support #128

Add twitter future support #128

Conversation

ghost commented Aug 3, 2017 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomjadams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 3, 2017 • edited

Codecov Report

ghost commented Aug 3, 2017

raulraja commented Aug 7, 2017

dominv commented Aug 7, 2017

peterneyens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterneyens Aug 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterneyens left a comment

Choose a reason for hiding this comment

dominv commented Aug 17, 2017

Choose a reason for hiding this comment

peterneyens commented Aug 17, 2017 • edited

ghost commented Aug 17, 2017

travisbrown commented Aug 17, 2017

peterneyens commented Aug 17, 2017 • edited

ghost commented Aug 3, 2017 •

edited by ghost

codecov-io commented Aug 3, 2017 •

edited

peterneyens Aug 15, 2017 •

edited

peterneyens commented Aug 17, 2017 •

edited

peterneyens commented Aug 17, 2017 •

edited