[Conjure Java Runtime] Part 26: Story of Thanatosis (Removing AtlasDB-Feign Entirely) #4561

jeremyk-91 · 2020-02-11T11:44:37Z

Goals (and why):

Remove the atlasdb-feign project from the codebase.
Generally leave people fewer complications to think about in the remoting stack (e.g. James thought that we could still fallback when beginning to write the Conjure migration). I thought I'd give a 1 hour stab at this, and it looks like it wasn't as hard as I thought.

Implementation Description (bullets):

Remove atlasdb-feign, fix compile and checkstyle issues that arise from that.

Testing (What was existing testing like? What have you done to improve it?):

Existing tests still pass.
The code compiling serves as verification that it isn't referenced any more.

Concerns (what feedback would you like?):

I wanted to eliminate RemotingClientConfig as well, but wasn't too happy with the API break/mess there.
Should the experiment running proxy code be retained?
There are a few trouble spots:
- Leader block mechanism to elide remote calls in a 1 node cluster used Feign but have now been switched to CJR. I've updated the failure mechanism to match, but is this correct?
- Benchmarking infrastructure had some fancy magic with FeignOkHttpClients, which of course isn't used in CJR. Does this still work?
Are there some edge cases that I've missed out?

Where should we start reviewing?: Top down, looking at places which are actually +/- as opposed to just - is probably a good approach.

Priority (whenever / two weeks / yesterday): Whenever

changelog-app · 2020-02-11T11:44:44Z

Generate changelog in `changelog/@unreleased`

Type

Description

The atlasdb-feign project is no longer published. Remoting within AtlasDB has already been going through Conjure Java Runtime for multiple versions. Direct users of classes in this project will need to find suitable alternatives - please contact the AtlasDB team if this is an issue.

Check the box to generate changelog(s)

Generate changelog entry

jkozlowski · 2020-02-11T11:59:15Z

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

-                () -> AtlasDbFeignTargetFactory.DEFAULT.createProxyWithFailover(serverListConfig, type, parameters),
-                type,
-                parameters);
+        return VersionSelectingClients.instrumentWithClientVersionTag(


I haven't read the PR, but we need to make sure we don't miss the code in ExperimentRunningProxy:

this.refreshingExperimentalServiceSupplier = Suppliers.memoizeWithExpiration( experimentalServiceSupplier::get, CLIENT_REFRESH_INTERVAL.toMinutes(), TimeUnit.MINUTES);

Without this we loose the 30 minute client reload thing, meaning we'll get the lockups back again.

felixdesouza

Looks mainly good, couple of questions.

I do think we should push through with the deletion/stop deserializing remoting config. It's used in 2 places internally, and if we're keeping the ALWAYS_USE_CONJURE bit, then they will continue to work no problem. But just to clean up/require less cognitive load when perusing through the remoting layer, we should remove the configs and the VersionSelectingClient.

felixdesouza · 2020-02-17T14:54:29Z

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

-                () -> AtlasDbFeignTargetFactory.DEFAULT.createProxyWithFailover(serverListConfig, type, parameters),
-                type,
-                parameters);
+        Supplier<T> clientFactory = () -> VersionSelectingClients.instrumentWithClientVersionTag(


can we remove VersionSelectingClients? I don't think it's relevant anymore.

felixdesouza · 2020-02-17T14:54:59Z

atlasdb-commons/src/main/java/com/palantir/common/proxy/SelfRefreshingProxy.java

+ * the existing instance. Once the refresh interval has expired, a new instance is not guaranteed to be created until
+ * a method is invoked on the proxy.
+ */
+public class SelfRefreshingProxy<T> extends AbstractInvocationHandler {


Nice, in my hack branch, I have the same thing, although a bit condensed, it's just the Suppliers.memoizeWithExpiration piece etc.

felixdesouza · 2020-02-17T14:56:28Z

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

-                            log.warn("Failed to read remote timestamp server ID", e);
-                            logAfter = LOGGING_INTERVAL;
+                    } catch (UnknownRemoteException e) {
+                        if (400 <= e.getStatus() && e.getStatus() <= 499) {


can you go into a bit more detail as to why it has to be done this way?

Sure, added a comment. Basically it looks like the treatment of an unmapped 4xx has changed: this was a ClientErrorException in the Feign world, but looks like that has been changed to be a UnknownRemoteException in the CJR world.

felixdesouza · 2020-02-17T14:58:14Z

atlasdb-remoting-api/src/main/java/com/palantir/atlasdb/config/AuxiliaryRemotingParameters.java

@@ -48,7 +48,11 @@ default boolean shouldSupportBlockingOperations() {
        return true;
    }

+    /**
+     * @deprecated Conjure is now always used, and supplying alternative configs is not possible.


I don't think this is true? still feels like you can have something that gets deserialised into a RemotingClientConfig no?

I think we should go through and purge the config stuff properly. Deprecations don't really do anything if we never remove them.

I also don't think we want to deprecate this anyway. Since if we do need to open up other configurations i.e. if we want to add some configurability for our own testing purposes, this is the right place to do it.

Starting from the config RemotingClientConfig, we can delete the two properties, and if we're super concerned we can do what we did in AtlasDbConfig, where as a class level annotation, we do @JsonIgnoreProperties({"maximumConjureRemotingProbability", "enableLegacyClientFallback"} and then we delete those properties.

We should probably add a RemotingClientConfigs.DEFAULT configuration, and then have the old thing RemotingClientConfigs.ALWAYS_USE_CONJURE refer to that, and change all of our references to that.

Then we can propagate the deletion of the java properties everywhere (i.e. and continue to get rid of VersionSelectingClients.

felixdesouza · 2020-02-17T14:58:44Z

timelock-agent/src/test/java/com/palantir/timelock/paxos/LeaderPingHealthCheckTest.java

@@ -188,7 +187,7 @@ private static HealthCheckPinger getMockOfPingableLeaderWherePingReturns(Set<Cli

    private static HealthCheckPinger getMockOfPingableLeaderWherePingThrows() {
        HealthCheckPinger mockLeader = mock(HealthCheckPinger.class);
-        when(mockLeader.apply(ALL_CLIENTS)).thenThrow(mock(AtlasDbRemoteException.class));
+        when(mockLeader.apply(ALL_CLIENTS)).thenThrow(mock(IllegalStateException.class));


Yep, sorry about this 😞

jeremyk-91

Fixed the easier-to-fix bits first, later part will come soon but have other priorities at the moment. Thanks for the review.

jeremyk-91 · 2020-02-24T18:40:08Z

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

-                            log.warn("Failed to read remote timestamp server ID", e);
-                            logAfter = LOGGING_INTERVAL;
+                    } catch (UnknownRemoteException e) {
+                        if (400 <= e.getStatus() && e.getStatus() <= 499) {


Sure, added a comment. Basically it looks like the treatment of an unmapped 4xx has changed: this was a ClientErrorException in the Feign world, but looks like that has been changed to be a UnknownRemoteException in the CJR world.

felixdesouza

I'd probably get rid of the extra instrumentation since we don't need it. But other than that looks good. Will leave it to you to merge when you're ready.

felixdesouza · 2020-03-10T17:19:25Z

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

                metricsManager.getTaggedRegistry(),
                ConjureJavaRuntimeTargetFactory.DEFAULT.createProxyWithQuickFailoverForTesting(
                        serverListConfig, type, parameters),
                type);
        return SelfRefreshingProxy.create(clientFactory, type);
    }
+
+    private static <T> T instrument(


iirc, instrumentation has always been separate from creating clients. i.e. ServiceCreator and in timelock (before and after my changes). Unless they're all changed, we're going to double report, and likely we'll have wrong metrics on our dashboards.

good catch!

Discussed offline: This is messy, because just removing will mean that we lose some metrics - not true that all ServiceCreator created things are instrumented. Will do as a follow up to this PR.

felixdesouza · 2020-03-10T17:21:11Z

atlasdb-remoting-api/src/main/java/com/palantir/atlasdb/config/RemotingClientConfigs.java

@@ -21,8 +21,5 @@ private RemotingClientConfigs() {
        // Constants
    }

-    public static final RemotingClientConfig ALWAYS_USE_CONJURE = ImmutableRemotingClientConfig.builder()


note to self/you: you'll have to change the places where this is referred to explicitly namely big internal product and it's related projects.

Yeah, that's right. Thanks

jeremyk-91 requested a review from gmaretic February 11, 2020 11:44

jkozlowski reviewed Feb 11, 2020

View reviewed changes

felixdesouza reviewed Feb 17, 2020

View reviewed changes

felixdesouza mentioned this pull request Feb 17, 2020

[Timelock OOMs] Memoized supplier with timeout client #4580

Merged

jeremyk-91 added 10 commits February 24, 2020 18:40

Remove atlasdb-feign

a5dca24

Some fixes

a43266d

Fix accidental compile break

3e65089

Fix the build

8e45df7

Fix broken tests

360fa74

Baseline

b7d5a02

Add generated changelog entries

cce2b36

Replacement for Exp Running Proxy

8730854

Unwrap ITEs

4cc4efc

bla

16e4e84

jeremyk-91 force-pushed the jkong/cjr-burn branch from efdf886 to 16e4e84 Compare February 24, 2020 18:41

CR: simple changes

af136ef

jeremyk-91 commented Feb 24, 2020

View reviewed changes

jeremyk-91 added 2 commits March 10, 2020 17:09

Remove config options

a15bb06

Update RCC

0150b62

felixdesouza approved these changes Mar 10, 2020

View reviewed changes

jeremyk-91 added 3 commits March 10, 2020 17:52

kein tags

0e0e951

Merge branch 'develop' into jkong/cjr-burn

a9fe210

Optimize imports

1454e93

jeremyk-91 merged commit 5fa9117 into develop Mar 10, 2020

delete-merged-branch bot deleted the jkong/cjr-burn branch March 10, 2020 19:07

pkoenig10 mentioned this pull request Jul 31, 2021

ScheduledRefreshingClient is no longer necessary #3017

Closed

pkoenig10 mentioned this pull request Dec 20, 2021

Remove remoting2 dependency #5830

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Conjure Java Runtime] Part 26: Story of Thanatosis (Removing AtlasDB-Feign Entirely) #4561

[Conjure Java Runtime] Part 26: Story of Thanatosis (Removing AtlasDB-Feign Entirely) #4561

jeremyk-91 commented Feb 11, 2020

changelog-app bot commented Feb 11, 2020 •

edited by jeremyk-91

Loading

jkozlowski Feb 11, 2020

felixdesouza left a comment

felixdesouza Feb 17, 2020

felixdesouza Feb 17, 2020

felixdesouza Feb 17, 2020

jeremyk-91 Feb 24, 2020

felixdesouza Feb 17, 2020

felixdesouza Feb 17, 2020

felixdesouza Feb 17, 2020

jeremyk-91 Feb 24, 2020

jeremyk-91 left a comment

jeremyk-91 Feb 24, 2020

felixdesouza left a comment

felixdesouza Mar 10, 2020

jeremyk-91 Mar 10, 2020

jeremyk-91 Mar 10, 2020

felixdesouza Mar 10, 2020

jeremyk-91 Mar 10, 2020

[Conjure Java Runtime] Part 26: Story of Thanatosis (Removing AtlasDB-Feign Entirely) #4561

[Conjure Java Runtime] Part 26: Story of Thanatosis (Removing AtlasDB-Feign Entirely) #4561

Conversation

jeremyk-91 commented Feb 11, 2020

changelog-app bot commented Feb 11, 2020 • edited by jeremyk-91 Loading

Generate changelog in changelog/@unreleased

Choose a reason for hiding this comment

felixdesouza left a 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

jeremyk-91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixdesouza left a 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 Feb 11, 2020 •

edited by jeremyk-91

Loading

Generate changelog in `changelog/@unreleased`