reuse concurrency limiter state when reloading host channels #2413

bjlaub · 2024-11-08T17:08:57Z

Before this PR

The recent changes to implement DNS node discovery came with the drawback that when domain name resolution changes are detected, we reload channels causing all internal channel state to be discarded and recreated anew. This means that concurrency limiter state (i.e. the current limit values encapsulated within a CautiousIncreaseAggressiveDecreaseConcurrencyLimiter instance) for a per-host channel are lost and we need to restart the congestion control algorithm.

This can have some implications in both directions: if the limiter state has reduced the concurrency significantly because the server asked us to slow down, we may end up bombarding the host with more requests than it can handle when a reload happens; if the limiter state has increased concurrency significantly, we will artificially start limiting request concurrency and have to slowly work our way back up to better throughput.

After this PR

This change introduces a ChannelState capable of holding internal state to be retained across channel reloads. In this change, we add a cache mapping TargetUri to a ChannelState instance, where each ChannelState holds an instance of a CautiousIncreaseAggressiveDecreaseConcurrencyLimiter. This allows a ConcurrencyLimitedChannel for a specific TargetUri to be created anew but re-use existing limiter state for that particular host.

Note that this change only affects per-host channels, not per-endpoint channels (which are also concurrency limited, but interact with a QueuedChannel in a much different way). A follow-on PR will address retaining limiter state for per-endpoint channels upon reload.

==COMMIT_MSG==
reuse concurrency limiter state when reloading host channels
==COMMIT_MSG==

Possible downsides?

changelog-app · 2024-11-08T17:09:02Z

Generate changelog in `changelog/@unreleased`

What do the change types mean?

feature: A new feature of the service.
improvement: An incremental improvement in the functionality or operation of the service.
fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
and external consumers of the service's API (e.g. customer-written software or integrations).
deprecation: Advertises the intention to remove service functionality without any change to the
operation of the service itself.
manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
performing database surgery, ...) at the time of upgrade for it to succeed.
migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

❗The break and manual task changelog types will result in a major release!
🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
✨ All others will result in a minor version release.

Type

Description

reuse concurrency limiter state when reloading host channels

Check the box to generate changelog(s)

Generate changelog entry

carterkozak · 2024-11-12T16:22:54Z

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

+        }
+    }
+
+    static final class StateHolder {


disclaimer: naming is hard, and I'm not very good at it. Take my comments with a grain of salt!

Thoughts on extracting this to its own file, and renaming to ChannelState? "Holder" makes me think it's using the old initialize-on-demand holder pattern.

If we extract to its own file, the key type could make sense as a static nested class, since the outer class provides context as to what it's a key for e.g. ChannelState.Key.

carterkozak · 2024-11-12T16:23:30Z

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

+    @Value.Immutable
+    interface ConcurrencyLimitedChannelState {
+        CautiousIncreaseAggressiveDecreaseConcurrencyLimiter hostLimiter();
+    }


I think we can put the CautiousIncreaseAggressiveDecreaseConcurrencyLimiter directly into the state, since it's the only piece of data we're tracking

carterkozak · 2024-11-12T16:30:39Z

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

+    private static final StateHolderKey<ConcurrencyLimitedChannelState> STATE_HOLDER_KEY =
+            new StateHolderKey<>(ConcurrencyLimitedChannelState.class, ConcurrencyLimitedChannel::createState);


II think we should include HOST in the name, since this is creating a host-specific limiter

carterkozak · 2024-11-12T21:29:53Z

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

+        T cast(final Object value) {
+            return valueClass.cast(value);
+        }
+
+        Supplier<T> getFactory() {
+            return factory;
+        }


These can be private

carterkozak · 2024-11-12T21:31:01Z

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

+        }
+    }
+
+    @SuppressWarnings("DangerousIdentityKey")


We could implement equals and hashCode on Key, delegating to super to make this explicit (and avoid the suppression)

carterkozak · 2024-11-12T21:34:26Z

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

+        if (state.containsKey(key)) {
+            return key.cast(state.get(key));
+        } else {
+            T value = key.getFactory().get();
+            Preconditions.checkNotNull(value, "state factory cannot produce a null value");
+            state.put(key, value);
+            return value;
+        }


I don't think we expect this to be a hot path, so we can use something like:

return key.cast(Preconditions.checkNotNull( state.computeIfAbsent(key, keyValue -> keyValue.getFactory().get()), "state factory cannot produce a null value"));

carterkozak · 2024-11-14T14:42:05Z

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

-            }));
+            LimitedChannel nodeSelectionChannel =
+                    new SupplierChannel(cf.uris().map(new Function<List<TargetUri>, LimitedChannel>() {
+                        private final Map<TargetUri, ChannelState> state = new HashMap<>();


Let's make this a ConcurrentHashMap for safety, that way we don't need to think about it

carterkozak · 2024-11-14T14:44:57Z

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

+                            Set<TargetUri> toRemove = state.keySet().stream()
+                                    .filter(uri -> !targetUris.contains(uri))
+                                    .collect(Collectors.toSet());
+                            toRemove.forEach(state::remove);


I think we can replace this with state.keySet().retainAll(targetUris);

👍 this is voodoo that i didn't know existed

carterkozak · 2024-11-14T14:46:59Z

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

+                                    .filter(uri -> !targetUris.contains(uri))
+                                    .collect(Collectors.toSet());
+                            toRemove.forEach(state::remove);
+                            targetUris.forEach(uri -> state.putIfAbsent(uri, new ChannelState()));


Perhaps computeIfAbsent(uri, _uri -> new ChannelState()) to avoid creating ChannelState instances for keys that are already in the map?

bjlaub · 2024-11-14T16:31:01Z

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

@@ -39,13 +40,20 @@
 final class ConcurrencyLimitedChannel implements LimitedChannel {
    private static final SafeLogger log = SafeLoggerFactory.get(ConcurrencyLimitedChannel.class);

+    @VisibleForTesting
+    static final ChannelState.Key<CautiousIncreaseAggressiveDecreaseConcurrencyLimiter> HOST_SPECIFIC_STATE_KEY =


this is visible for the purposes of ConcurrencyLimitedChannelTest#testReuseCachedLimiterState_host, which may not be a particularly useful test. I'm somewhat indifferent, we could delete that test and make this private.

carterkozak reviewed Nov 12, 2024

View reviewed changes

carterkozak reviewed Nov 14, 2024

View reviewed changes

bjlaub commented Nov 14, 2024

View reviewed changes

bjlaub changed the title ~~WIP thing to save state when reloading channels~~ reuse concurrency limiter state when reloading host channels Nov 14, 2024

bjlaub marked this pull request as ready for review November 14, 2024 16:43

bjlaub added the merge when ready label Nov 18, 2024

Brian Laub added 13 commits November 18, 2024 12:23

WIP thing to save state when reloading channels

0a1491a

refactor

8fd7739

only retain state for per host channel

a28b15e

move state class to separate file

0d8bc1c

use AIMD limiter class as the state object

f518185

spotless

477442a

pr feedback

2e39ee4

unit test for ChannelState

5e96680

add a test to ConcurrencyLimitedChannelTest

f2e84f7

spotless

3ed0561

wip test cache state on reload

fba3072

fix test

57e59bc

pr feedback

7cb1e69

bjlaub force-pushed the blaub/retain-state branch from 3d820e4 to 7cb1e69 Compare November 18, 2024 17:24

Add generated changelog entries

7093e86

carterkozak approved these changes Nov 18, 2024

View reviewed changes

bulldozer-bot bot merged commit 5fd0855 into develop Nov 18, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reuse concurrency limiter state when reloading host channels #2413

reuse concurrency limiter state when reloading host channels #2413

bjlaub commented Nov 8, 2024 •

edited

Loading

changelog-app bot commented Nov 8, 2024 •

edited by bjlaub

Loading

carterkozak Nov 12, 2024

bjlaub Nov 12, 2024

carterkozak Nov 12, 2024

bjlaub Nov 12, 2024

carterkozak Nov 12, 2024

bjlaub Nov 12, 2024

carterkozak Nov 12, 2024

carterkozak Nov 12, 2024

carterkozak Nov 12, 2024 •

edited

Loading

carterkozak Nov 14, 2024

carterkozak Nov 14, 2024

bjlaub Nov 14, 2024

carterkozak Nov 14, 2024

bjlaub Nov 14, 2024

		private static final StateHolderKey<ConcurrencyLimitedChannelState> STATE_HOLDER_KEY =
		new StateHolderKey<>(ConcurrencyLimitedChannelState.class, ConcurrencyLimitedChannel::createState);

reuse concurrency limiter state when reloading host channels #2413

reuse concurrency limiter state when reloading host channels #2413

Conversation

bjlaub commented Nov 8, 2024 • edited Loading

Before this PR

After this PR

Possible downsides?

changelog-app bot commented Nov 8, 2024 • edited by bjlaub Loading

Generate changelog in changelog/@unreleased

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carterkozak Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjlaub commented Nov 8, 2024 •

edited

Loading

changelog-app bot commented Nov 8, 2024 •

edited by bjlaub

Loading

Generate changelog in `changelog/@unreleased`

carterkozak Nov 12, 2024 •

edited

Loading