BlacklistingChannel probation and per-endpoint info #349
Conversation
Generate changelog in
|
dialogue-core/src/main/java/com/palantir/dialogue/core/BlacklistingChannel.java
Outdated
Show resolved
Hide resolved
| this.duration = duration; | ||
| this.ticker = ticker; | ||
| this.perEndpointBlacklistState = | ||
| Caffeine.newBuilder().maximumSize(1000).ticker(ticker).build(); |
There was a problem hiding this comment.
per-endpoint state feels a bit odd here. At the channel level I think we'd want to track health of the channel as a whole. I wonder if we could invert the relationship between channel and Endpoint to allow endpoints to be wrapped similarly to conjure-undertow?
There was a problem hiding this comment.
Is this something that you want to nail down before moving forwards with this PR? (We've already got a Cache<Endpoint, Limiter> in our concurrency limiters...)
This BlacklistingChannel is a key ingredient for an alternative node selection strategy I've been working on, but I'd really like to land this PR as it's not super usable as is.
| } | ||
|
|
||
| // I wish java had union types | ||
| interface BlacklistState {} |
There was a problem hiding this comment.
derive4j is your friend here
There was a problem hiding this comment.
So I actually really don't want to have the extra faff and tooling setup of derive4j - can you forgive these two purely implementation-detail classes?
|
👍 |
|
FWIW Channels#create doesn't currently use BlacklistingChannel - it was a piece of floating dead code... but maybe not for long ;) |
|
If we want to use blacklisting channel we’ll need to wire in eventing to attempt to flush the queue when channels are no longer blacklisted.
…-ck
On Feb 18, 2020, at 6:11 PM, iamdanfox ***@***.***> wrote:
FWIW Channels#create doesn't currently use BlacklistingChannel - it was a piece of floating dead code... but maybe not for long ;)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Ah right fair enough, as the concurrency limiter's behaviour only changes when a request goes in or comes back (which triggers the queued thing to schedule), whereas since blacklisting is time based, we could have a spike of requests that all get queued and if no further requests come in then they'd just sit on the queue forever. |
Before this PR
(It's way easier to review this when I have the
:simulationgraphs to demonstrate what's broken)After this PR
==COMMIT_MSG==
BlacklistingChannel keeps track of per-endpoint information and conservatively ramps up.
==COMMIT_MSG==
Possible downsides?