New entry-point: Dialogue.newClientPool() #398

iamdanfox · 2020-02-20T22:55:17Z

Before this PR

We need to give people an easy way to build multiple clients that all use the same connection pool under the hood.
There's quite a few of different little bits of API surface in dialogue, which makes it a little confusing for users to know what they're supposed to interact with
We don't currently cleanup or dispose of client resources when we're done
We also rely on the old c-j-r ClientConfiguration type everywhere

After this PR

==COMMIT_MSG==
A new Dialogue.newClientPool() method gives people a single clear entrypoint to the library.
==COMMIT_MSG==

It people build clients in a declarative way that people are familiar with from feign:

AsyncFooService foo = clients.get(AsyncFooService.class, listenableConfig);
AsyncOtherService other = clients.get(AsyncOtherService.class, listenableConfig);

// ... later, wc can call:
clients.close()

By getting conjure to generate enums, we can actually do this without using any sketchy reflection (no InvocationTargetExceptions anywhere!)

Obviously this doesn't have enough tests to merge yet, just wanted to see what you guys think of the idea before I sink too much time into it!

Possible downsides?

yet another refreshable implementation

iamdanfox · 2020-02-20T22:56:52Z

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

+ * Facilitates creating many clients which all share the same connection pool. Should only create one of these per
+ * server. Close it when your server shuts down to release resources.
+ */
+public interface ClientPool extends Closeable {


This is probably a reasonable place to start reading this PR if you want

iamdanfox · 2020-02-20T22:59:51Z

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

+import com.google.errorprone.annotations.MustBeClosed;
+import com.palantir.dialogue.ConjureRuntime;
+
+public final class Dialogue {


My dream is if we can make all the things people care about accessible from one top-level starting point, so they can just remember "the method is on Dialogue somewhere".

I think if that's the case it'd be better to return a ClientFactory that uses the provided ClientPool so that no user of Dialogue has to know anything about something called a ClientPool.

it's not so clear to me that this code should live in dialogue land rather than in wc?

@uschi2000 are you referring to just this one entrypoint or the whole PR?

I think the idea of a "singleton client factory", i.e., something that holds on to resources for the lifetime of a process, is something I'd expect in a server library, not a client library.

I think if we try to make all our libraries good enough to use on their own, then when we pull them into WC they'll still be good. I actually think a client library that doesn't give you a convenient way to release resources when you want to seems broken/incomplete.

To be clear this API doesn't know anything about the lifetime of a process, and there are no static singletons here. it would still be up to witchcraft to make sure there's one client pool and witchcrafts responsibility to close it at shutdown time.

Dialogue is designed to be a singleton, no?
Yes, it's a bug today that we don't close channels, but then why isn't it sufficient for this library to add Channel#close? I prefer hierarchical ownership (with delegating .close()) over complex "inside-out" resource management; is that not an option here?

markelliot · 2020-02-20T23:57:14Z

We need to give people an easy way to build multiple clients that all use the same connection pool under the hood.

Is this actually desirable?

markelliot · 2020-02-21T00:06:12Z

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

+     * will always fail if the specified uri is not listed in the latest version of the config. Somewhat dangerous
+     * because this has no limits / failover.
+     */
+    Channel rawHttpChannel(String uri, Listenable<DialogueConfig> config);


is this actually something that should be exposed?

Probably not to be honest - I started with this 3-member interface just to hash out in my head what the key building blocks were. Happy to push it down to package private.

markelliot · 2020-02-21T00:06:20Z

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

+     * Returns a channel for interacting with the given abstract upstream service, which routes traffic
+     * appropriately to the various available nodes. Live-reloaded every time the urls change.
+     */
+    Channel smartChannel(Listenable<DialogueConfig> config);


when does one use this instead of get?

iamdanfox · 2020-02-21T00:07:56Z

I think so, because the average WC server has 38 clients according to datadog, but I think some have much fewer (like maybe 2) and some have even more. This means any limits are gonna seem to apply quite differently. It also tries to provide similar-ish behaviour to our old c-j-r okhttp world, where there was just one static instance.

From the metric summary page in datadog (which shows you tag cardinality), we've got 26.7k witchcraft servers (reporting the jvm.safepoint.time metric) and 1,016,011 series of client.response.count. 1016011 / 26700 = 38

carterkozak · 2020-02-21T01:42:37Z

...ogue-apache-hc4-client/src/main/java/com/palantir/dialogue/hc4/DialogueApacheHttpClient.java

+                .getOrComputeIfAbsent(
+                        "one-off-client-construction",
+                        unused -> {
+                            return constructLiveReloadingClient(config); // we'll re-use this instance every time


I think we need a unique CloseableHttpClient instance for each ssl configuration -- each service can have different trust data. A few other prefs from the configuration are applied directly to the CloseableHttpClient so they will require unique instances

carterkozak · 2020-02-21T01:47:34Z

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

+     * Returns a channel for interacting with the given abstract upstream service, which routes traffic
+     * appropriately to the various available nodes. Live-reloaded every time the urls change.
+     */
+    Channel smartChannel(Listenable<DialogueConfig> config);


I think we should have some API separation between channel creation and using a channel to create a generated client (which would delegate to the first component)

carterkozak · 2020-02-21T01:52:45Z

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

+        return factory.construct(smartChannel, runtime);
+    }
+
+    private static <F> F getOnlyEnumConstant(Class<F> factoryClass) {


Suggested change

private static <F> F getOnlyEnumConstant(Class<F> factoryClass) {

private static <F extends Enum<F>> F getOnlyEnumConstant(Class<F> factoryClass) {

The only way I can get this to work is if I then make the argument Class<?>, which seems a bit less nice?

uschi2000 · 2020-02-21T13:19:09Z

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

+
+        Class<? extends HttpChannelFactory> getFactoryClass() {
+            try {
+                return (Class<? extends HttpChannelFactory>) Class.forName(className);


why do we need reflection here?

i.e., why not Supplier<Channel>?

I didn't want people who depend on dialogue-core to transitively get dialogue-apache, dialogue-java, dialogue-okhttp etc.

Might be able to doing this with a compileOnly dependency if you'd prefer?

I'm not a huge fan of runtime dependencies like this. might indicate that the decomp is a little off, and maybe also that this should live in the server repo?

iamdanfox · 2020-02-24T14:05:12Z

Closing out as I'm resuscitating my PR to do this in WC

iamdanfox added 21 commits February 20, 2020 16:07

braindump

6fdf2a0

Javadoc my intentions

f096c15

ClientPoolImpl

76409a9

WIP

9c7f454

Factory#construct requires a Channel

d7a8274

Wire up to Channels#create

14f0e5b

Appease static analysis

7e448ac

ConstructUsing

3f50051

SharedResources

3341441

DialogueApacheHttpClient

824d4ef

Use RefreshingChannelFactory

468a7fa

Clear warnings when we can't live reload something

37fcf2f

implement refreshable again

59aae08

SharedResourcesImpl

d3a797a

Test passes from intellij

ab8f8fd

No cycle

afa7b94

No cycle

308b034

Show a sample async client working

1e80e53

checkstyle

655e485

Use enums instead

bd6faee

Some more comments

273a0c3

iamdanfox added the no changelog label Feb 20, 2020

iamdanfox commented Feb 20, 2020

View reviewed changes

iamdanfox requested review from carterkozak and ferozco February 20, 2020 22:57

iamdanfox commented Feb 20, 2020

View reviewed changes

markelliot reviewed Feb 21, 2020

View reviewed changes

carterkozak reviewed Feb 21, 2020

View reviewed changes

uschi2000 reviewed Feb 21, 2020

View reviewed changes

ListenableValue is test-only

147de40

iamdanfox closed this Feb 24, 2020

iamdanfox mentioned this pull request Feb 25, 2020

ApacheHttpChannels exposes a Closeable instance #430

Merged

iamdanfox deleted the dfox/facade branch May 12, 2020 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New entry-point: Dialogue.newClientPool() #398

New entry-point: Dialogue.newClientPool() #398

iamdanfox commented Feb 20, 2020 •

edited

Loading

iamdanfox Feb 20, 2020

iamdanfox Feb 20, 2020

markelliot Feb 21, 2020

uschi2000 Feb 21, 2020

iamdanfox Feb 21, 2020

uschi2000 Feb 21, 2020

iamdanfox Feb 21, 2020 •

edited

Loading

uschi2000 Feb 21, 2020

markelliot commented Feb 20, 2020 •

edited

Loading

markelliot Feb 21, 2020

iamdanfox Feb 21, 2020 •

edited

Loading

markelliot Feb 21, 2020

iamdanfox commented Feb 21, 2020

carterkozak Feb 21, 2020

carterkozak Feb 21, 2020

carterkozak Feb 21, 2020

iamdanfox Feb 21, 2020

uschi2000 Feb 21, 2020

uschi2000 Feb 21, 2020

iamdanfox Feb 21, 2020

uschi2000 Feb 21, 2020

iamdanfox commented Feb 24, 2020

	private static <F> F getOnlyEnumConstant(Class<F> factoryClass) {
	private static <F extends Enum<F>> F getOnlyEnumConstant(Class<F> factoryClass) {

New entry-point: Dialogue.newClientPool() #398

New entry-point: Dialogue.newClientPool() #398

Conversation

iamdanfox commented Feb 20, 2020 • edited Loading

Before this PR

After this PR

Possible downsides?

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

iamdanfox Feb 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markelliot commented Feb 20, 2020 • edited Loading

Choose a reason for hiding this comment

iamdanfox Feb 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamdanfox commented Feb 21, 2020

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

iamdanfox commented Feb 24, 2020

iamdanfox commented Feb 20, 2020 •

edited

Loading

iamdanfox Feb 21, 2020 •

edited

Loading

markelliot commented Feb 20, 2020 •

edited

Loading

iamdanfox Feb 21, 2020 •

edited

Loading