Add methods to obtain named per-host clients

[pkoenig10]

Before this PR

There are some useful capabilities not currently supported by the per-host channels:

• The ability to order the channels, in a way that is consistent across node and Dialogue versions
• The ability to compare channel targets before and after a configuration reload
• The ability to determine if a channel target is ourselves

After this PR

The PerHostClientFactory now provides two additional methods, getNamedPerHostChannels and getNamedPerHost, that return channels keyed by PerHostTarget. PerHostTarget allows callers to introspect the target and determine whether that target refers to themselves.

[changelog-app]

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

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

PerHostClientFactory now provides methods to obtain per-host clients keyed by a target identifier.
Add methods to obtain named per-host clients.

Check the box to generate changelog(s)

• Generate changelog entry

[pkoenig10]

We had originally discussed designing PerHostTarget to look like:

class PerHostTarget {
    // An opaque hash of the uri and resolvedAddress
    String name;
    boolean isSelf;
}

The problem with this approach is that the same logical target may produce different names across Dialogue versions. For example, if we add a new field to TargetUri that gets incorporated into the hash. This is bad for a couple reasons:

• Consumers using different Dialogue version will disagree on the sorted order of these channels.
• Consumers may attempt to persist these names and later compare them against names produced by a different Dialogue version.

The only capabilities we require from PerHostTarget are that it has a consistent ordering for the same logical target across Dialogue version.

This PR instead accomplishes this by making TargetUri implement Comparable<TargetUri>. If we add new fields to TargetUri, we can implement compareTo such that the order remains consistent.

[carterkozak]

I wanted to avoid leaking uri-info to callers, as dialogue aims to provide an abstraction that makes such info unnecessary. However, I don't think it will be possible to provide enough precision for our users without making this data available in some form. TargetUri is used to direct requests to individual upstreams (with greater specificity than a URI), so any future additions to abstract away something like nodes behind a reverse proxy (no plans to do this, just speculating about potential future changes) would have to be represented in TargetUri, which would be similarly helpful here -- so there shouldn't be any need to expose similar fields from a new type here. I think retaining the PerHostTarget wrapper around TargetUri buys us some freedom to add new metadata for per-host-channels in the future if needs be later on.

The isSelf accessor was an attempt to avoid avoid leaking these details, but I think if we do expose the TargetUri, we needn't expose isSelf (that sort of check could become library functionality elsewhere, but it isn't required for the dialogue feature imo). The TargetUri will allow consumers to be more accurate in their own isSelf check, for instance when N services are running on the same host, the caller is likely to know which port it's listening on, in order to provide further differentiation from other services on the same host.

[carterkozak]

I think we want this to fail loudly/catastrophically if something goes wrong

[pkoenig10]

Is this safe? There's no guarantee that the TargetUri values are unique.

[carterkozak]

Hmm, I suppose an example might look something like this:

uris:
  - https://target/api
  - https://target/api

In such a case, I think I'd prefer if we pruned out duplicates earlier on, otherwise we risk effectively doubling the concurrency limits on repeated values. I think we can improve things across the board by updating ReloadingClientFactory.getTargetUris with something like this to retain order, but filter duplicates:

-return ImmutableList.copyOf(targetUris);
+return ImmutableSet.copyOf(targetUris).toList();

[carterkozak]

Slight preference to internally represent things like this using the immutables type for clarity

Suggested change

	Refreshable<Map<PerHostTarget, Channel>> perHostChannels = configurationForService(serviceName)
	Refreshable<ImmutableMap<PerHostTarget, Channel>> perHostChannels = configurationForService(serviceName)

This also has the minor upside of making code like 288 a bit simpler:

-ImmutableList.copyOf(channels.values())
+channels.values().asList()

[pkoenig10]

This runs into the problem mentioned below in #2192 (comment).

You can't return a Refreshable<ImmutableMap> from a method that returns Refreshable<Map>.

[carterkozak]

slight preference to avoid stream() where possible

ImmutableMap.copyOf(Maps.transformValues(
    channels,
    channel -> Reflection.callStaticFactoryMethod(clientInterface, entry.getValue(), params.runtime())));

[pkoenig10]

Why is this code better? We have to allocate some temporary variables in either case.

We're already using streams in a number of places in this class.

[carterkozak]

Streams are difficult for hotspot to optimize, and I find to-map stream collectors a bit more difficult to read at a glance (this makes it more clear to devs that the keys aren't being modified, and the transformValues call makes it clear to the internals that the resulting ImmutableMap can be pre-sized).

Almost certainly won't make a measurable difference in practice, my goal in the comment is to share how I'm thinking about this, it's entirely up to you whether or not you'd like to change the snippet :-)

[carterkozak]

Thoughts on delegation here?

return getNamedPerHost(clientInterface).map(result -> result.values().asList());

[pkoenig10]

Should we make the existing methods default methods on PerHostClientFactory? Or would you prefer to keep the implementation here?

[pkoenig10]

Delegation would require me to change the return type declared in PerHostClientFactory to be ImmutableMap, which probably isn't what we want to do here.

It's also nice to have one less refreshable subscriber here.

[carterkozak]

Delegation would require me to change the return type declared in PerHostClientFactory to be ImmutableMap, which probably isn't what we want to do here.

Ah, we didn't handle PECS (Producer Extends Consumer Super) very well on the PerHostClientFactory existing interface methods -- ideally we may have declared something like Refreshable<? extends List<? extends Channel>> getPerHostChannels(); for greater flexibility in the implementation, however that does force callers who don't use the var keyword to declare a fair bit more cruft.

Happy to leave this as is

[carterkozak]

lgtm, thanks!

Invalidated by push of 5878c99

[svc-autorelease]

Released 3.123.0