spotify / completable-futures

Utilities for working with futures in Java 8
Apache License 2.0
393 stars 51 forks source link

Add CompletableFutures.poll() #41

Closed mbruggmann closed 8 years ago

mbruggmann commented 8 years ago

This periodically polls an external task until it returns a non-empty result. My specific use-case here is talking to the BigQuery API, where I have to send repeated HTTP requests until the result becomes available.

codecov-io commented 8 years ago

Current coverage is 100%

Merging #41 into master will not change coverage

@@            master       #41   diff @@
========================================
  Files            4         4          
  Lines           51        61    +10   
  Methods          0         0          
  Messages         0         0          
  Branches         3         3          
========================================
+ Hits            51        61    +10   
  Misses           0         0          
  Partials         0         0          

Powered by Codecov. Last updated by 335cfeb...565a7b3

adcaes commented 8 years ago

👍 Neat

mattnworb commented 8 years ago

I was just implementing something similar in a project, this looks great 👍

Is Duration.ofMillis(2) in the test of any special value?

mbruggmann commented 8 years ago

@mattnworb There is nothing particular about the 2ms duration in the test (except being >0).

spkrka commented 8 years ago

:+1: Nice!

dflemstr commented 8 years ago

I really would like to have this functionality, great idea! 👌

It's a pity that we need to depend on mockito though, would be nice if we could avoid that. I made some API change suggestions above but we could also go for some other solution; as long as we don't have to mock anything I think we'd be on the right track!

mbruggmann commented 8 years ago

@dflemstr Do you have a specific reason why you wouldn't want to use mocks for the tests? Or is the issue with mockito in particular?

dflemstr commented 8 years ago

Basically this. Of course there are cases when mocks are needed (like when using external systems, like mocking some result of conn.execute("SELECT really_complex_thing();")), but I don't think we will ever reach that point in this library since it's only dealing with in-memory stuff.

mbruggmann commented 8 years ago

@dflemstr Makes sense. I removed one unnecessary use of mockito, but it's still there to verify the interaction with the ScheduledExecutorService. Having that complexity abstracted in completable-futures and covered by tests sounds to me like the lesser evil than pulling in a mocking framework. We can keep an eye on not over-using mocks in PRs?

dflemstr commented 8 years ago

:+1: I think this is good now, but agreed that we should be mindful about not using mocking in the future.