Closed benblack86 closed 7 years ago
Merging #537 into master will increase coverage by
0.12%
. The diff coverage is83.33%
.
@@ Coverage Diff @@
## master #537 +/- ##
==========================================
+ Coverage 84.71% 84.84% +0.12%
==========================================
Files 98 98
Lines 4293 4295 +2
Branches 331 334 +3
==========================================
+ Hits 3637 3644 +7
+ Misses 656 651 -5
Impacted Files | Coverage Δ | |
---|---|---|
...main/scala/colossus/protocols/redis/RedisOps.scala | 94.96% <ø> (-0.04%) |
:arrow_down: |
...ain/scala/colossus/protocols/http/HttpClient.scala | 88.88% <ø> (-1.12%) |
:arrow_down: |
...cala/colossus/protocols/memcache/MemcacheOps.scala | 80% <ø> (-0.56%) |
:arrow_down: |
...ain/scala/colossus/protocols/http/HttpServer.scala | 58.82% <0%> (-9.93%) |
:arrow_down: |
.../main/scala/colossus/protocols/redis/package.scala | 28.57% <100%> (+11.9%) |
:arrow_up: |
...in/scala/colossus/protocols/memcache/package.scala | 100% <100%> (ø) |
:arrow_up: |
...c/main/scala/colossus/protocols/http/package.scala | 71.42% <100%> (+38.09%) |
:arrow_up: |
...lossus/src/main/scala/colossus/service/Async.scala | 54.54% <100%> (+4.54%) |
:arrow_up: |
...s/src/main/scala/colossus/service/ServiceDSL.scala | 85.41% <100%> (+2.08%) |
:arrow_up: |
.../scala/colossus/metrics/collectors/Histogram.scala | 92.64% <0%> (+0.73%) |
:arrow_up: |
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 3cb941f...00bf0f2. Read the comment docs.
Yeah most of that stuff isn't needed anymore now that users cannot directly create ServiceClient
and FutureClient
. I will examine this more in depth later today, but at first glance it seems fine. I want to make sure we have some tests around creating clients, including load balancers, but I'm pretty sure everything that I expect to work will still work.
As for API changes, yes this looks like it will break binary compatibility and should go in 0.10. On that note we should probably spend some time to actually scope out what we want to go in 0.10 (I don't think it should be too much, want to start aiming for faster releases).
I've started to add what I think should be in 0.10 (i.e. lib upgrades). https://github.com/tumblr/colossus/milestones?state=open
I will talk to the team on their thoughts. This refactor (getting rid of most internal implicits) will go in 0.10 also, although still waiting on comments from other people.
Going to split up and put into 0.10
I was trying to understand how the client setup logic works and in particular how the
CodecClientFactory
was creating the client. However, I was getting stuck understanding how it was all glued together due tobaseFactory
,lifter
, andbuilder
being implicits. I was like, where are these implicits coming from!?! After a while I realized that some of them there being defined in different files, but under the same namespace so where in scope.To bring more clarity to the code base, I've turning the implicits for
CodecClientFactory
andClientFactories
into explicit parameters. Thumbs up, thumbs down?I then went crazy and moved the lifter implementations to where they are used and cut out some default objects that weren't being used. In the case of http, I moved the error routing logic to where it is being used. I'm guessing this breaks the rule of no API changes so I should leave those for 0.10?
Thoughts?