Closed sschepens closed 5 years ago
tests are failing on circle but work ok on my computer, investigating
Merging #85 into master will increase coverage by
0.08%
. The diff coverage is100%
.
@@ Coverage Diff @@
## master #85 +/- ##
============================================
+ Coverage 94.39% 94.47% +0.08%
- Complexity 131 134 +3
============================================
Files 13 14 +1
Lines 517 525 +8
Branches 46 46
============================================
+ Hits 488 496 +8
Misses 21 21
Partials 8 8
Impacted Files | Coverage Δ | Complexity Δ | |
---|---|---|---|
...nvoyproxy/controlplane/server/DiscoveryServer.java | 96.49% <100%> (+0.19%) |
14 <1> (+1) |
:arrow_up: |
...roxy/controlplane/server/DefaultExecutorGroup.java | 100% <100%> (ø) |
2 <2> (?) |
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 c56c848...0743dee. Read the comment docs.
@snowp DefaultExecutorGroup
is mostly copypasted from Netty DefaultEventExecutorGroup
, wouldn't you think it's probably better to add a dependency on netty-common
and use that?
We would avoid having to test and mantain this class.
I would actually go in the other direction: provide the minimum interface necessary here and a very simple implementation, maybe something that just returns the same fixed thread executor each time. That way you can use a netty implementation if you want without bringing all that complexity into the control plane API.
Using a netty implementation would be as simple as doing
class NettyExecutorGroup implements ExecutorGroup {
EventExecutorGroup nettyGroup;
@Override public Executor next() {
return nettyGroup.next();
}
}
@snowp thanks for the comments, totally agree.
I committed a different, simpler approach, moved ExecutorGroup
and DefaultExecutorGroup
to server
to RR Executor
s per stream.
Also DefaultExecutorGroup
now defaults to the old behavior by just returning directExecutor
.
This approach is much simpler and touches less things, please review.
We don't have to solve this now, but it would be good to document this a bit more. I get the mechanics of it from a high-level, but I don't think I have a full picture of the different types of ExecutorGroup impls that people would want in the real world. Are there use-cases where you would want certain request streams to have an affinity for a particular Executor? Or is it mostly just direct executor vs. thread pool, with the pool size and pool entry chooser (e.g. netty's PowerOfTwoEventExecutorChooser) being the most interesting parts of the latter?
@joeyb I think it would mostly be just direct executor vs. thread pool, What I can see is people probably trying to reuse netty's worker groups, I would personally like that. I can see little benefit from having stream affinity, even so, right now we have little to no context for taking these decisions.
@sschepens - Cool, makes sense. If the need for more context comes up, then we can cross that bridge when we get there.
@sschepens, @snowp - FYI, I cut 0.1.12
, which includes this change.
@joeyb thanks!
Supersedes #75
Added concept of
ExecutorGroup
mostly based on netty'sEventExecutorGroup
for distributingExecutor
s to streams in a round robin fashion.