tumblr / colossus

I/O and Microservice library for Scala
Apache License 2.0
1.14k stars 96 forks source link

Service client #610

Closed benblack86 closed 6 years ago

benblack86 commented 7 years ago

Behold, the new service client MVP. It looks like this:

val hostManager = HostManager()
hostManager.addHost("localhost", 6379)
hostManager.addHost("localhost", 6379)
hostManager.addHost("localhost", 6379)

val redisClient = Redis.futureClient(hostManager)

The default behavior is to try every host with exponential backoff. However, you could configure something different, for example:

val hostManager = HostManager(
  LBRetryPolicy(
    replacement = Random,
    backoff = LinearBackoff(1.second),
    attempts = MaxAttempts(3)
  )
)

The possibilities are endless.

It's missing some implementation details and tests, but my main concern is around removing a client from the host manager. When a client is removed, it is disconnected, meaning that any requests that are currently using it will be affected. Maybe this is fine (making the assumption retries is turned on), maybe we should disconnect after a few seconds, or maybe the disconnect should only happen when we know for certain that no request is using it (not sure how feasible that is).

Thoughts?

@DanSimon @amotamed @aliyakamercan @nsauro @dxuhuang @jlbelmonte

codecov-io commented 7 years ago

Codecov Report

Merging #610 into develop will decrease coverage by 1.16%. The diff coverage is 4.61%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #610      +/-   ##
===========================================
- Coverage    84.88%   83.72%   -1.17%     
===========================================
  Files           99      101       +2     
  Lines         4452     4515      +63     
  Branches       361      369       +8     
===========================================
+ Hits          3779     3780       +1     
- Misses         673      735      +62
Impacted Files Coverage Δ
.../src/main/scala/colossus/service/HostManager.scala 0% <0%> (ø)
...ain/scala/colossus/service/LoadBalanceClient.scala 0% <0%> (ø)
...s/src/main/scala/colossus/service/ServiceDSL.scala 83.67% <0%> (-1.75%) :arrow_down:
...lossus/src/main/scala/colossus/service/Async.scala 26.08% <20%> (-23.92%) :arrow_down:

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 1f5b26d...6878702. Read the comment docs.

benblack86 commented 6 years ago

Going to close this and open after some changes. The problem with the HostManager concept is that it is hard to configure through config. You would probably need to namespace the host manager and then have a separate section for client config and host manager config, which will be confusing for a colossus user.