Client respect `Node-Selection-Strategy` response header #688

ferozco · 2020-04-28T23:18:29Z

Before this PR

Clients would declare (or default to) a static node selection strategy. Clients would have to guess which node selection strategy was the right one to use for a given service, resulting in a leaky abstraction since they would have to know implementation details of the service they were calling to choose the right one.

After this PR

==COMMIT_MSG==
Clients respect Node-Selection-Strategy response header
==COMMIT_MSG==

Servers are now able to instruct clients on which strategy to use by adding Node-Selection-Strategy response header. The header is structured similarly to Content-Type as a ; delimited list of strategies, ordered by preference. The current supported strategies are:

PIN_UNTIL_ERROR
PIN_UNTIL_ERROR_WITHOUT_RESHUFFLE
BALANCED

The expectation is that for a given server a single node selection strategy will be defined.

The hope is that by consolidating node selection strategy selection into the server we will see improved perf across the fleet and clients will have to think less about the services they communicate with.

Currently the NodeSelectionStrategySelector decides which node selection strategy to use, basically the first known strategy from the most recent response.

Possible downsides?

This raises some questions about how we handle client creation internally, since we currently create separate DialogueChannel's (and underlying Apache client) if two logic services have different static client configuration (including node selection strategy) but they will converge to the same strategy if the server responds with Node-Selection-Strategy header.

There is also currently no way for the client to override the server suggested strategy, some may view that as a feature rather than a bug however 🤷‍♂️

simulation/src/test/resources/report.md

carterkozak · 2020-04-29T16:52:05Z

dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueNodeSelectionStrategy.java

+    UNKNOWN;
+
+    static List<DialogueNodeSelectionStrategy> fromHeader(String header) {
+        return Splitter.on(",").splitToList(header).stream()


We can extract out the splitter to a static field, and include trimming whitespace: Splitter.on(',').trimResults().omitEmptyStrings()

dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueNodeSelectionStrategy.java

carterkozak · 2020-04-29T16:55:17Z

dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueNodeSelectionStrategy.java

+
+    private static DialogueNodeSelectionStrategy safeValueOf(String value) {
+        try {
+            return valueOf(value.trim().toUpperCase());


Do we want to support mixed case strategies?

The Http spec only required header names be case insensitive so its up to each implementer to decide how it interprets the values. For simplicity I figured it was fine to be completely case insensitive.

dialogue-core/src/main/java/com/palantir/dialogue/core/NodeSelectionStrategyChannel.java

carterkozak · 2020-04-29T17:10:07Z

...ogue-core/src/main/java/com/palantir/dialogue/core/DefaultNodeSelectionStrategySelector.java

+import java.util.stream.Collectors;
+
+@SuppressWarnings("NullAway")
+public final class DefaultNodeSelectionStrategySelector implements NodeSelectionStrategySelector {


It would be helpful to mark a meter each time a strategy change occurs, possibly including the strategy we're moving to.

carterkozak · 2020-04-29T17:16:45Z

dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueChannel.java

-                ticker,
-                channelName));
-
+        nodeSelectionStrategy.updateChannels(ImmutableList.copyOf(limitedChannelByUri.values()));


We could keep things simpler by putting our complete trust in the most recent response we've received, that way the nodeSelectionStrategy wouldn't need to wrap individual uri channels or be notified of uri updates.

No real proposal here, just weighing implementing this in a more naive way to begin with.

Ya I had thought of that originally, but it would lead to pretty bad behaviour during rolling upgrades with RR/Balanced. I think the extra bit of complexity is worth handling the case

carterkozak · 2020-04-29T17:17:20Z

dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueChannel.java

-                        channels, random, tick, config.taggedMetricRegistry(), channelName);
-        }
-        throw new SafeRuntimeException(
-                "Unknown NodeSelectionStrategy", SafeArg.of("unknown", config.nodeSelectionStrategy()));


DialogueChannel is much easier to follow without the node selection logic 👍

dialogue-core/src/main/java/com/palantir/dialogue/core/NodeSelectionStrategyChannel.java

dialogue-core/src/main/java/com/palantir/dialogue/core/NodeSelectionStrategySelector.java

dialogue-core/src/main/java/com/palantir/dialogue/core/NodeSelectionStrategyChannel.java

dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueNodeSelectionStrategy.java

...-core/src/test/java/com/palantir/dialogue/core/DefaultNodeSelectionStrategySelectorTest.java

…ectionStrategySelector.java Co-Authored-By: Carter Kozak <ckozak@ckozak.net>

…e-selection

changelog-app · 2020-04-29T21:16:41Z

Generate changelog in `changelog/@unreleased`

Type

Description

Clients respect Node-Selection-Strategy response header

Check the box to generate changelog(s)

Generate changelog entry

carterkozak

I like it, thanks Felipe!
@iamdanfox Have you had a chance to read over this? This is a substantial enough change that I'd like to hold off merging until you've had a chance to review as well.

carterkozak · 2020-04-30T15:54:30Z

dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueNodeSelectionStrategy.java

+
+    private static DialogueNodeSelectionStrategy safeValueOf(String value) {
+        String normalizedValue = value.toUpperCase();
+        if (PIN_UNTIL_ERROR.name().equals(normalizedValue)) {


meganit: We can avoid toUpperCase in favor of equalsIgnoreCase

* README for node selection strategy * Consistent capitalization

…e-selection

…ntir/dialogue into fo/server-side-node-selection

…e-selection

…ntir/dialogue into fo/server-side-node-selection

iamdanfox · 2020-04-30T16:51:00Z

dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueNodeSelectionStrategy.java

+    PIN_UNTIL_ERROR,
+    PIN_UNTIL_ERROR_WITHOUT_RESHUFFLE,
+    BALANCED,
+    UNKNOWN;


I think we can probably ditch the UNKNOWN variant here, as this library is the source of truth for what strategies exist?

But you could have an old client communicating with a new server and hence receive an unknown strategy

Also can we put a little comment saying why this is copy&pasted version of the regular remoting-api NodeSelectionStrategy? (iirc this was so that we could try out implementation details without having to promote them to a full user-selectable option?)

old client communicating with a new server and hence receive an unknown strategy

in this case though we just ignore it right? i'm not sure what the unknown variant really buys us.

It becomes important if you decide to have more complicated logic (what was previously implemented) for determining the node selection strategy to use. If you really don't like it, i'm happy to remove it

iamdanfox · 2020-04-30T17:07:28Z

dialogue-core/src/main/java/com/palantir/dialogue/core/NodeSelectionStrategyChannel.java

+
+        LimitedChannel channel();
+
+        ImmutableList<LimitedChannel> hostChannels();


~~why are hostChannels saved here rather than just using an instance field?~~ ah nvm it's live reloading again isn't it?

Ya we need to keep track of the host channels so that we can recreate the underlying channel in response to a server's node-selection-strategy response

dialogue-core/src/main/java/com/palantir/dialogue/core/NodeSelectionStrategyChannel.java

dialogue-core/src/main/metrics/dialogue-core-metrics.yml

dialogue-core/src/main/java/com/palantir/dialogue/core/NodeSelectionStrategyChannel.java

dialogue-core/src/main/java/com/palantir/dialogue/core/NodeSelectionStrategySelector.java

…ialogue into fo/server-side-node-selection

iamdanfox · 2020-05-01T13:02:22Z

dialogue-core/src/test/java/com/palantir/dialogue/core/NodeSelectionStrategyChannelTest.java

+    private static void setResponse(LimitedChannel mockChannel, Optional<String> header) {
+        Mockito.clearInvocations(mockChannel);
+        Mockito.reset(mockChannel);
+        Response resp = mock(Response.class);


would prefer to use TestResponse.withheader rather than mocking, but not a big deal

iamdanfox · 2020-05-01T13:03:49Z

dialogue-core/src/test/java/com/palantir/dialogue/core/NodeSelectionStrategyChannelTest.java

+    void handles_one_to_many_uri_update() {
+        channel.updateChannels(ImmutableList.of(channel1));
+        channel.updateChannels(ImmutableList.of(channel1, channel2));
+    }


Somewhat concerned that these tests could pretty easily vacuously pass, but maybe we have enough unit tests elsewhere that prove we can make requests correctly after a reload?

iamdanfox

Ok let's give it a shot! V. excited to be able to get our Balanced implementation out to more of the fleet. I expect it's gonna be a bit weird when blue-greening between nodes that suggest different Node-Selection-Strategies, but this should be pretty transient and I guess we can watch the metrics to see when it happens :)

svc-autorelease · 2020-05-01T13:58:27Z

Released 1.32.0

forozco added 4 commits April 28, 2020 17:47

allow servers to define client node selection strategy

1cb5e2e

tests and null away

f0581e8

ignore leading unknown strategies

234dc9d

update simulations

39bbaa6

probot-autolabeler bot added the autorelease label Apr 28, 2020

ferozco requested review from iamdanfox and carterkozak April 28, 2020 23:18

ferozco commented Apr 28, 2020

View reviewed changes

simulation/src/test/resources/report.md Outdated Show resolved Hide resolved

forozco added 3 commits April 28, 2020 20:34

pinned channel is reused across updates

77cc246

NSS selector uses score to determine strategy

b944fdc

cleanup

7a8f236

ferozco marked this pull request as ready for review April 29, 2020 16:23