[Conjure Java Runtime] Part 10: Drive a truck through the AtlasDbHttpClients API #4264

jeremyk-91 · 2019-09-26T18:35:33Z

Goals (and why):

Enable live reloadable configuration of whether requests should go through the legacy or CJR path.
Avoid having the various methods all end with String userAgent, boolean limitPayloadSize, RemotingClientConfig config

Implementation Description (bullets):

Convert the aforementioned parameters into an AuxiliaryRemotingParameters struct
Fix the API breaks everywhere
Plumb the config through as far as a client talking to timelock is concerned, which is probably where we'll start this off.

Testing (What was existing testing like? What have you done to improve it?):
Not much. Existing tests still work (some were changed as they asserted over-specific things IMO).

Concerns (what feedback would you like?):

This PR is kind of large, so I probably missed something or introduced some unnecessary duplication when fixing the fallout of the API break.
Is the API break overly aggressive?
I changed the expectations of some tests. I think these are reasonable, but worth a second pass.

Where should we start reviewing?:

AuxiliaryRemotingParameters

Priority (whenever / two weeks / yesterday): this week would be great!

changelog-app · 2019-09-26T18:35:38Z

Generate changelog in `changelog/@unreleased`

Type

Description

AtlasDbHttpClients now expects an AuxiliaryRemotingParameters struct which encapsulates information about whether to use payload limiting, the user agent, and any additional remoting client configuration (such as that that may be provided by the AtlasDB library). These were previously expected as primitives.

To replicate previous behaviour, users should create an AuxiliaryRemotingParameters struct, possibly as follows:

AuxiliaryRemotingParameters.builder()
        .userAgent(UserAgents.tryParse(userAgentString))
        .shouldLimitPayload(false)
        .remotingClientConfig(remotingClientConfigSupplier)
        .build();

userAgent and remotingClientConfig are optional; shouldLimitPayload is compulsory.

Check the box to generate changelog(s)

Generate changelog entry

felixdesouza · 2019-09-26T19:11:02Z

atlasdb-config/src/main/java/com/palantir/atlasdb/factory/TransactionManagers.java

@@ -210,6 +213,11 @@ boolean lockImmutableTsOnReadOnlyTransactions() {

    abstract String userAgent();


mark this as deprecated?

felixdesouza · 2019-09-26T19:15:00Z

atlasdb-config/src/main/java/com/palantir/atlasdb/factory/TransactionManagers.java

@@ -258,10 +266,11 @@ public static TransactionManager createInMemory(Set<Schema> schemas) {
        AtlasDbConfig config = ImmutableAtlasDbConfig.builder().keyValueService(new InMemoryAtlasDbConfig()).build();
        return builder()
                .config(config)
-                .userAgent(UserAgents.DEFAULT_USER_AGENT)
+                .userAgent(AtlasDbRemotingConstants.DEFAULT_USER_AGENT.toString())


why do you have to specify both? I think it should be an error if this is done.

Given we're still keeping userAgent around, and have a default structuredUserAgent, this is sadly a little tricky to do

felixdesouza · 2019-09-26T19:15:50Z

atlasdb-config/src/main/java/com/palantir/atlasdb/factory/TransactionManagers.java

@@ -1005,7 +1022,7 @@ private static LockAndTimestampServices createRawLeaderServices(
                    ServiceCreator.createTrustContext(leaderConfig.sslConfiguration()),
                    Iterables.getOnlyElement(leaderConfig.leaders()),
                    PingableLeader.class,
-                    userAgent);
+                    userAgent.toString());


unconverted

Changed the underlying method to take a UserAgent object.

felixdesouza · 2019-09-26T19:17:59Z

atlasdb-feign/src/main/java/com/palantir/atlasdb/config/AuxiliaryRemotingParameters.java

+
+    @Value.Default
+    default UserAgent userAgent() {
+        return AtlasDbRemotingConstants.DEFAULT_USER_AGENT;


hmm, not sure how I feel about a default user agent

Pairing: we removed the default.

felixdesouza · 2019-09-26T19:19:41Z

atlasdb-feign/src/main/java/com/palantir/atlasdb/config/RemotingClientConfig.java

+public interface RemotingClientConfig {
+    RemotingClientConfig DEFAULT = ImmutableRemotingClientConfig.builder().build();
+
+    default double maximumV2Probability() {


probably something a bit more descriptive of the change?

Yep, changed

felixdesouza · 2019-09-27T13:36:45Z

atlasdb-config/src/main/java/com/palantir/atlasdb/http/AtlasDbHttpClients.java

@@ -121,18 +123,16 @@ private AtlasDbHttpClients() {
                        trustContextCreator,
                        proxySelectorCreator,
                        type,
-                        userAgent,
-                        limitPayload),
+                        clientParameters),
                        DEFAULT_TARGET_FACTORY.getClientVersion()),
                ImmutableInstanceAndVersion.of(DEFAULT_TARGET_FACTORY.createLiveReloadingProxyWithFailover(


dunno whyI missed this last time, but the wrapper seems redundant, since the instance and the version is on the factory, unless I'm missing something

Pairing: we pushed InstanceAndVersion down into the TargetFactory layer

felixdesouza · 2019-09-27T13:43:31Z

atlasdb-feign/src/main/java/com/palantir/atlasdb/http/TargetFactory.java

@@ -32,31 +33,27 @@
            Optional<TrustContext> trustContext,


I think the option to not retry can fold into AuxiliaryRemotingParameters?

Pairing: yep, folded this in to AuxiliaryRemotingParameters

felixdesouza · 2019-09-27T13:50:28Z

atlasdb-feign/src/main/java/com/palantir/atlasdb/http/TargetFactory.java


    <T> T createProxyWithFailover(
            Optional<TrustContext> trustContext,
            Optional<ProxySelector> proxySelector,
            Collection<String> endpointUris,
            Class<T> type,
-            String userAgent,
-            boolean limitPayloadSize);
+            AuxiliaryRemotingParameters parameters);

    <T> T createLiveReloadingProxyWithFailover(
            Supplier<ServerListConfig> serverListConfigSupplier,
            Function<SslConfiguration, TrustContext> trustContextCreator,


we should also kill, these two parameters, i.e. trustContextCreator and proxySelectorCreator. We only have one impl, and it's never anything different. All the stuff we need is inside ServerListConfig

Pairing: We removed these functions.

felixdesouza · 2019-09-27T13:51:35Z

atlasdb-feign/src/main/java/com/palantir/atlasdb/http/TargetFactory.java


    <T> T createProxy(
            Optional<TrustContext> trustContext,
            String uri,
            Class<T> type,
-            String userAgent,
-            boolean limitPayloadSize);
+            AuxiliaryRemotingParameters parameters);

    <T> T createProxyWithFailover(


we should then make this a default that just takes in a ServerListConfig and then returns a static supplier to the createLiveReloadingProxyWithFailover.

with this, we only have two methods to implement 💃

Pairing: Sadly it goes the other way (the live reloading one falls back to this) so it's not as straightforward

felixdesouza · 2019-09-27T13:52:50Z

changelog/@unreleased/pr-4264.v2.yml

+            .build();
+    ```
+
+    `userAgent` and `remotingClientConfig` are optional; `shouldLimitPayload` is compulsory.


not sure I agree about userAgent being optional

Yep, we made it compulsory (I'll need to change the docs).

jeremyk-91 added 7 commits September 26, 2019 17:45

checkpoint i

de82007

Drive a truck through the APIs

2565da0

Fix compiile breaks

248eec6

Merge conflicts and breaks

a606cc5

Fix more breaks

4fc49a8

Add generated changelog entries

2cdc23f

Add generated changelog entries

fae14c1

jeremyk-91 requested a review from felixdesouza September 27, 2019 09:48

jeremyk-91 mentioned this pull request Sep 27, 2019

[Conjure Java Runtime] Part 12: Conjure Java Runtime Target Factory #4266

Merged

felixdesouza reviewed Sep 27, 2019

View reviewed changes

jeremyk-91 added 3 commits September 27, 2019 17:01

Pairing session

c85c21e

Pairing session 2

faa3e1e

Other CR comments

407eeef

felixdesouza approved these changes Sep 27, 2019

View reviewed changes

jeremyk-91 added 5 commits September 27, 2019 17:36

Update changelog to reflect structured User Agents

5f93697

Merge branch 'develop' into jkong/proxy-api-wreck-2

3841b82

Fix some tests

e7866b1

bleh

97e3013

Fix test

65e9e65

jeremyk-91 merged commit 305c9d7 into develop Sep 30, 2019

delete-merged-branch bot deleted the jkong/proxy-api-wreck-2 branch September 30, 2019 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Conjure Java Runtime] Part 10: Drive a truck through the AtlasDbHttpClients API #4264

[Conjure Java Runtime] Part 10: Drive a truck through the AtlasDbHttpClients API #4264

jeremyk-91 commented Sep 26, 2019

changelog-app bot commented Sep 26, 2019 •

edited by jeremyk-91

Loading

felixdesouza Sep 26, 2019

felixdesouza Sep 26, 2019

jeremyk-91 Sep 27, 2019

felixdesouza Sep 26, 2019

jeremyk-91 Sep 27, 2019

felixdesouza Sep 26, 2019

jeremyk-91 Sep 27, 2019

felixdesouza Sep 26, 2019

jeremyk-91 Sep 27, 2019

felixdesouza Sep 27, 2019

jeremyk-91 Sep 27, 2019

felixdesouza Sep 27, 2019

jeremyk-91 Sep 27, 2019

felixdesouza Sep 27, 2019

jeremyk-91 Sep 27, 2019

felixdesouza Sep 27, 2019

felixdesouza Sep 27, 2019

jeremyk-91 Sep 27, 2019

felixdesouza Sep 27, 2019

jeremyk-91 Sep 27, 2019

		@@ -210,6 +213,11 @@ boolean lockImmutableTsOnReadOnlyTransactions() {

		abstract String userAgent();

[Conjure Java Runtime] Part 10: Drive a truck through the AtlasDbHttpClients API #4264

[Conjure Java Runtime] Part 10: Drive a truck through the AtlasDbHttpClients API #4264

Conversation

jeremyk-91 commented Sep 26, 2019

changelog-app bot commented Sep 26, 2019 • edited by jeremyk-91 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

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

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

changelog-app bot commented Sep 26, 2019 •

edited by jeremyk-91

Loading

Generate changelog in `changelog/@unreleased`