-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Thriftmux -> 213 #794
Thriftmux -> 213 #794
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a small nit, LGTM otherwise. I'm surprised this was so easy!
@@ -13,6 +13,7 @@ import com.twitter.finagle.param.{ | |||
import com.twitter.finagle.server.{Listener, StackServer, StdStackServer} | |||
import com.twitter.finagle.service.{ResponseClassifier, RetryBudget} | |||
import com.twitter.finagle.stats.{ExceptionStatsHandler, StatsReceiver} | |||
import com.twitter.finagle | |||
import com.twitter.finagle.thrift.{ClientId => _, _} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we instead provide an alias for ClientId that's FinagleClientId?
Codecov Report
@@ Coverage Diff @@
## develop #794 +/- ##
===========================================
- Coverage 71.83% 71.83% -0.01%
===========================================
Files 859 859
Lines 25601 25603 +2
Branches 1872 1868 -4
===========================================
+ Hits 18390 18391 +1
- Misses 7211 7212 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great, thanks @martijnhoekstra!
thanks @martijnhoekstra this got merged here: 47ee31f |
Problem
No 2.13 cross-building for finagle-thrift, finagle-mux and finagle-thrift-mux
Solution
Cross-build.