-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support behaviour changes based on URIs with a 'mesh-' prefix #1048
Conversation
Generate changelog in
|
dialogue-core/src/main/java/com/palantir/dialogue/core/Config.java
Outdated
Show resolved
Hide resolved
dialogue-core/src/main/java/com/palantir/dialogue/core/Config.java
Outdated
Show resolved
Hide resolved
@@ -81,4 +137,8 @@ default void check() { | |||
SafeArg.of("numUris", rawConfig().uris().size())); | |||
} | |||
} | |||
|
|||
static String stripMeshPrefix(String input) { | |||
return input.startsWith(MESH_PREFIX) ? input.substring(MESH_PREFIX.length()) : input; |
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.
the apache client allows custom protocol handlers, so we can avoid any url rewriting, and unsafe messages will include the mesh prefix for easier debugging. wdyt?
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.
So i guess this decision kinda impacts whether this mesh-
flag should be allowed to affect stuff inside whatever 'ChannelFactory' we're using (i.e. apache-land), or whether it's a purely for enabling/disabling things in dialogue-core.
Personally, I'd prefer to keep this dialogue internal, because if we're asking more people to make decisions based on this then I think we should find a way of exposing it in a nice typed way.
.taggedMetricRegistry( | ||
VersionedTaggedMetricRegistry.create(rawConfig().taggedMetricRegistry())) | ||
.build(); | ||
} | ||
|
||
@Value.Derived | ||
default MeshMode mesh() { |
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 keep all of the meshmode stuff internal to DialogueChannel? i.e. perform the computation internally instead of having a derived field on the config object?
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.
Sure we could, but then there's a possibility of constructing a Config object which is in a non-sensical state - i.e. has mesh-
prefixed uris, but doesn't have MeshMode.USE_EXTERNAL_MESH
set. The reason I think it's appropriate & desirable here is to emphasise to people that this isn't an extra degree of freedom in how you build a Config object, instead it's just purely derived from the URIs that you already put in. This is an example of the "make illegal states unrepresentable" pattern.
Otherwise, to keep this safety, we'd just have to put in a Value.Check that computes the mesh mode.
Third option is to not even have the mesh() param on this object, which would then require plumbing it in everywhere that we currently pass Config cf
.
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.
At least within the scope of this PR it seems like there are only 2 places that actually care about the mesh mode stuff, and in both cases they didn't previous require the entire Config
object to be initialized. So it doesn't seem like we're saving much by pushing the mesh-mode stuff into the config object and it just exposes additional API on the Config
.
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.
Config is package private, so the only people who will be affected by this decision will be us / future maintainers of dialogue.
Back in april I found dialogue channel had grown to become really hard to read, so condensed all of the 'inputs' into this one Config object. That enables the pattern where Channels are created with this convention:
HostMetricsChannel.create(cf, channel, uri)
QueuedChannel.create(cf, endpoint, limited)
DeprecationWarningChannel.create(cf, channel, endpoint);
TimingEndpointChannel.create(cf, channel, endpoint);
In each of these cases, Config cf
is only plumbed to the static factory method, so the actual Channel's constructor has only the bits it really needs (e.g. a clock or a channelname string etc), which makes it convenient to supply only exactly what you need for unit-testing.
It might seem minor, but i do feel kinda strongly about this... I think this convention helps us minimize plumbing, keeping the top level DialogueChannel as concise as possible so that you can follow which channels are wired into which other channels without getting distracted.
For future readers, @whickman shared the relevant quip where all these options were evaluated was https://palantir.quip.com/WrjDAIQZNRbT |
Before this PR
@whickman and the Network Infra team are working on adopting a service mesh in k8s, and when they do, we don't really want to have two layers of retrying (once in the mesh and once in dialogue) as this would result in multiplicatively slower failures.
After this PR
==COMMIT_MSG==
Prefixing uris with
mesh-
now turns off all retries.==COMMIT_MSG==
Some open questions
Alternative approaches were considered:
Possible downsides?