Use a Builder to create Transaction Managers #2459

tpetracca · 2017-10-10T17:07:46Z

Goals (and why): Resolves #2164

Implementation Description (bullets):

Concerns (what feedback would you like?): There are breaking developer changes here. I want to make even more of these in a better version of #2457; should we just rip the band-aid off all at once?

Where should we start reviewing?: TransactionMangers.java

Priority (whenever / two weeks / yesterday): Ideally today or tomorrow. I'm kind of considering this blocking #2457 which I need a real version of for internal backup service ASAP.

This change is

tpetracca · 2017-10-10T17:10:34Z

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

        KeyValueService kvs = ProfilingKeyValueService.create(rawKvs, config.getKvsSlowLogThresholdMillis());
        kvs = SweepStatsKeyValueService.create(kvs,
                new TimelockTimestampServiceAdapter(lockAndTimestampServices.timelock()));
        kvs = TracingKeyValueService.create(kvs);
-        kvs = AtlasDbMetrics.instrument(KeyValueService.class, kvs, MetricRegistry.name(KeyValueService.class));
+        kvs = AtlasDbMetrics.instrument(
+                KeyValueService.class, kvs, MetricRegistry.name(KeyValueService.class, options.derivedUserAgent()));


regression: need to delete the user agent

codecov-io · 2017-10-10T18:56:31Z

Codecov Report

Merging #2459 into develop will increase coverage by 0.01%.
The diff coverage is 53.73%.

@@              Coverage Diff              @@
##             develop    #2459      +/-   ##
=============================================
+ Coverage      60.05%   60.07%   +0.01%     
- Complexity      4658     4661       +3     
=============================================
  Files            859      859              
  Lines          40025    40057      +32     
  Branches        4071     4071              
=============================================
+ Hits           24039    24063      +24     
- Misses         14521    14534      +13     
+ Partials        1465     1460       -5

Impacted Files	Coverage Δ	Complexity Δ
.../palantir/atlasdb/server/AtlasDbServiceServer.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...ntir/atlasdb/console/module/AtlasCoreModule.groovy	`16.66% <0%> (+0.59%)`	`0 <0> (ø)`	⬇️
...ain/java/com/palantir/atlasdb/factory/Leaders.java	`95.06% <100%> (ø)`	`0 <0> (ø)`	⬇️
.../palantir/atlasdb/factory/TransactionManagers.java	`77.6% <55.17%> (-7.6%)`	`5 <0> (ø)`
...palantir/leader/proxy/AwaitingLeadershipProxy.java	`70.11% <0%> (-2.3%)`	`0% <0%> (ø)`
...alantir/lock/client/LockRefreshingLockService.java	`58.82% <0%> (-1.48%)`	`10% <0%> (-1%)`
...tion/impl/AbstractSerializableTransactionTest.java	`85.75% <0%> (+0.54%)`	`34% <0%> (ø)`	⬇️
...ain/java/com/palantir/paxos/PaxosStateLogImpl.java	`87.28% <0%> (+1.69%)`	`0% <0%> (ø)`	⬇️
...ain/java/com/palantir/paxos/PaxosProposerImpl.java	`91.66% <0%> (+3.33%)`	`0% <0%> (ø)`	⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7dec4c...37b81fc. Read the comment docs.

schlosna

👍 overall I like this much better than existing

schlosna · 2017-10-10T21:18:24Z

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

+    abstract Set<Schema> schemas();
+
+    @Value.Default
+    Consumer<Object> env() {


nit: should we rename this? env has felt weird outside context of dropwizard

I call it registrar or resourceRegistrar in other places where I've done similar things

schlosna · 2017-10-10T21:19:26Z

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

+    public static class Builder extends ImmutableTransactionManagers.Builder {
+        public SerializableTransactionManager buildSerializable() {
+            return build().serializable();
+        }


do we need to expose buildSnapshot?

we could? this got me to feature parity

yeah I think we're good with just Serializable. that's all the current API gives you

gsheasby · 2017-10-11T17:06:50Z

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

-     * Create a {@link SerializableTransactionManager} with provided configurations, {@link Schema},
-     * and an environment in which to register HTTP server endpoints.
-     */
-    public static SerializableTransactionManager create(


Can we keep these methods around, but mark them as deprecated? Seems nicer to me than breaking the world, and allows us to add some kind of lotion to the bandaid before ripping it off - with some time, the bandaid might fall painlessly off on its own.

(OK, I think I've taken the metaphor far enough)

gsheasby · 2017-10-11T17:09:56Z

Would like to avoid breaks at this time if at all possible, especially as we are trying to push a release to the internal behemoth ASAP.

tpetracca · 2017-10-11T19:25:07Z

@gsheasby basically only willing to "deprecate" if we decide up-front at what version we will remove and then actually do it then; so let's decide on that so I can add it to the release notes

tpetracca · 2017-10-11T20:19:53Z

Breaks have been rolled back and release notes updated. Arbitrarily chose 0.70, can change that to whatever we want when we release.

nziebart

LGTM aside from some minor comments

nziebart · 2017-10-12T05:25:17Z

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

@@ -62,22 +62,22 @@ private Leaders() {
     * Creates a LeaderElectionService using the supplied configuration and
     * registers appropriate endpoints for that service.
     */
-    public static LeaderElectionService create(Environment env, LeaderConfig config) {


any reason to change this to a Consumer? it's a little less obvious what it is now

(wouldn't be opposed to renaming the class EndpointRegistrar

Mostly it just seemed unnecessary? the interface is exactly that of a Consumer. And I've had issues in the past where I needed to pull atlasdb-config sadness on to my classpath just to get access to that interface (though on 2nd thought I could have worked around that).

Also makes it match how TimelockAgent works

nziebart · 2017-10-12T05:28:40Z

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

            KeyValueService kvs,
            SerializableTransactionManager transactionManager,
            SweepTaskRunner sweepRunner,
            BackgroundSweeperPerformanceLogger sweepPerfLogger,
-            Supplier<SweepBatchConfig> sweepBatchConfig,
+            com.google.common.base.Supplier<SweepBatchConfig> sweepBatchConfig,


ick. can these just be java suppliers?

this wasn't a change; you'll notice I swapped the import from guava to java to prevent un-suspecting contributors such as myself from adding more guava things without realizing. We can move these to java in a separate PR

nziebart · 2017-10-12T05:30:40Z

docs/source/release_notes/release-notes.rst

+    *    - |deprecated| |improved|
+         - ``SerializableTransactionManager`` is now created via an immutable builder instead of a long list of individual arguments. Use ``TransactionManagers.builder()``
+           to get the builder and once completely configured, build the transaction manager via the builder's ``.buildSerializable()`` method.
+           The existing ``create`` methods are deprecated and will be removed in a future 0.70 release.


how about we give a date here instead. November 15th? (about a month)

gsheasby

This certainly looks a lot nicer than what we had before, but I'm wondering if we can't pull out the value-type-ness of this class into TransactionManagerConfig. Then the usage would look like:

TransactionManagerConfig config = TransactionManagerConfig.builder()
                .config(config)
                .schemas(ETE_SCHEMAS)
                .registrar(environment.jersey()::register)
                .build();
TransactionManager manager = TransactionManagers.buildSerialiable(config);

Thoughts, @nziebart?

gsheasby · 2017-10-17T16:47:02Z

atlasdb-config/readme.md

-    sslSocketFactory,
-    atlasSchema,
-    (resource) -> {});
+SerializableTransactionManager tm = TransactionManagers.builder()


I had no idea this file existed... at some point we should move it into actual docs.

gsheasby · 2017-10-17T16:56:30Z

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

@@ -105,23 +107,64 @@
 import com.palantir.timestamp.TimestampStoreInvalidator;
 import com.palantir.util.OptionalResolver;

-public final class TransactionManagers {
-
+@Value.Immutable


it's kind of weird to think of this class as a value type...

I think might be less strange to reason that all of these fields are TransactionManagerOptions, and that is the value type here.

tpetracca · 2017-10-17T22:39:42Z

gsheasby

well, ok then

tpetracca added do not merge pr: work in progress labels Oct 10, 2017

tpetracca commented Oct 10, 2017

View reviewed changes

tpetracca force-pushed the tomp/tms-builder branch 2 times, most recently from bc77ba7 to b899cca Compare October 10, 2017 20:38

tpetracca removed do not merge pr: work in progress labels Oct 10, 2017

tpetracca requested review from schlosna and nziebart October 10, 2017 20:58

jeremyk-91 self-requested a review October 10, 2017 21:16

schlosna reviewed Oct 10, 2017

View reviewed changes

tpetracca mentioned this pull request Oct 10, 2017

Require executor to be passed to lock refreshing timelock #2466

Closed

gsheasby added the pr: external label Oct 11, 2017

gsheasby reviewed Oct 11, 2017

View reviewed changes

schlosna approved these changes Oct 11, 2017

View reviewed changes

nziebart reviewed Oct 12, 2017

View reviewed changes

gsheasby self-assigned this Oct 16, 2017

gsheasby reviewed Oct 17, 2017

View reviewed changes

gsheasby assigned nziebart and unassigned gsheasby Oct 17, 2017

gsheasby approved these changes Oct 18, 2017

View reviewed changes

gsheasby added the blocked on merge conflict label Oct 18, 2017

gsheasby assigned tpetracca and unassigned nziebart Oct 18, 2017

introduce immutable options parameter for TM creation

481c9fc

tpetracca added 11 commits October 18, 2017 13:41

Clean up TransactionManagers via breaks

e45ffda

fix metrics user agent regression

38a252d

move to builder

dba2391

fix readme and changelog

9822553

un-break the APIs

c2b2145

more un-breaking

de7e00d

update release notes

c6047f0

rename env to registrar

cb5290d

checkstyle

07afb64

maybe fix docs issue

b474f5c

use date for deprecation break

37b81fc

tpetracca force-pushed the tomp/tms-builder branch from 6fb75bc to 37b81fc Compare October 18, 2017 17:47

gsheasby merged commit 9d02b7f into develop Oct 19, 2017

tpetracca mentioned this pull request Oct 19, 2017

Deprecate transaction manager creation methods that don't specify user agents #2528

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a Builder to create Transaction Managers #2459

Use a Builder to create Transaction Managers #2459

tpetracca commented Oct 10, 2017 •

edited by jboreiko

Loading

tpetracca Oct 10, 2017

codecov-io commented Oct 10, 2017 •

edited

Loading

schlosna left a comment

schlosna Oct 10, 2017

tpetracca Oct 10, 2017

schlosna Oct 10, 2017

tpetracca Oct 10, 2017

nziebart Oct 12, 2017

gsheasby Oct 11, 2017

gsheasby commented Oct 11, 2017

tpetracca commented Oct 11, 2017

tpetracca commented Oct 11, 2017

nziebart left a comment

nziebart Oct 12, 2017

nziebart Oct 12, 2017

tpetracca Oct 12, 2017

tpetracca Oct 12, 2017

nziebart Oct 12, 2017

tpetracca Oct 12, 2017

nziebart Oct 12, 2017

gsheasby left a comment

gsheasby Oct 17, 2017

gsheasby Oct 17, 2017

gsheasby Oct 17, 2017

tpetracca commented Oct 17, 2017

gsheasby left a comment

Use a Builder to create Transaction Managers #2459

Use a Builder to create Transaction Managers #2459

Conversation

tpetracca commented Oct 10, 2017 • edited by jboreiko Loading

Choose a reason for hiding this comment

codecov-io commented Oct 10, 2017 • edited Loading

Codecov Report

schlosna 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

gsheasby commented Oct 11, 2017

tpetracca commented Oct 11, 2017

tpetracca commented Oct 11, 2017

nziebart 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

gsheasby 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

tpetracca commented Oct 17, 2017

gsheasby left a comment

Choose a reason for hiding this comment

tpetracca commented Oct 10, 2017 •

edited by jboreiko

Loading

codecov-io commented Oct 10, 2017 •

edited

Loading