Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

make user agent required #2542

Merged
merged 5 commits into from
Oct 23, 2017
Merged

make user agent required #2542

merged 5 commits into from
Oct 23, 2017

Conversation

tpetracca
Copy link
Contributor

@tpetracca tpetracca commented Oct 23, 2017

Goals (and why): Resolves #2528

Implementation Description (bullets): Require the previously optional userAgent method in the TransactionManagers builder. Remove the callingClass option.

Concerns (what feedback would you like?): Technically this is a breaking change, but we only just introduced this API last release and I doubt anyone is using yet. And either way people really should be doing this no matter what. The old create methods still exist and are still deprecated.

Where should we start reviewing?: TransactionManagers

Priority (whenever / two weeks / yesterday):


This change is Reviewable

@codecov-io
Copy link

codecov-io commented Oct 23, 2017

Codecov Report

Merging #2542 into develop will decrease coverage by 0.01%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             develop   #2542      +/-   ##
============================================
- Coverage      60.02%     60%   -0.02%     
+ Complexity      4659    4042     -617     
============================================
  Files            860     860              
  Lines          40121   40112       -9     
  Branches        4070    4070              
============================================
- Hits           24081   24069      -12     
- Misses         14572   14573       +1     
- Partials        1468    1470       +2
Impacted Files Coverage Δ Complexity Δ
...ntir/atlasdb/console/module/AtlasCoreModule.groovy 16.66% <ø> (ø) 0 <0> (ø) ⬇️
.../palantir/atlasdb/server/AtlasDbServiceServer.java 0% <0%> (ø) 0 <0> (ø) ⬇️
.../palantir/atlasdb/factory/TransactionManagers.java 77.6% <66.66%> (-0.09%) 5 <0> (ø)
...in/java/com/palantir/paxos/PaxosAcceptorState.java 78.94% <0%> (-10.53%) 0% <0%> (ø)
.../java/com/palantir/util/debug/StackTraceUtils.java 51.12% <0%> (-3.14%) 22% <0%> (ø)
...main/java/com/palantir/paxos/PaxosLearnerImpl.java 87.8% <0%> (-2.44%) 0% <0%> (ø)
...ain/java/com/palantir/paxos/PaxosAcceptorImpl.java 73.21% <0%> (ø) 0% <0%> (ø) ⬇️
...n/java/com/palantir/lock/impl/LockServiceImpl.java 82.95% <0%> (+0.35%) 0% <0%> (-91%) ⬇️
...m/palantir/atlasdb/sweep/SpecificTableSweeper.java 91.12% <0%> (+0.9%) 23% <0%> (-4%) ⬇️
...ain/java/com/palantir/paxos/PaxosStateLogImpl.java 87.28% <0%> (+1.69%) 0% <0%> (ø) ⬇️
... and 2 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 554dc2d...87e9154. Read the comment docs.

Copy link
Contributor

@hsaraogi hsaraogi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one small suggestion. Thanks!

@@ -48,6 +49,7 @@ public void run(AtlasDbServerConfiguration config, Environment env) throws Excep
TransactionManager transactionManager = TransactionManagers.builder()
.config(config.getAtlas())
.registrar(env.jersey()::register)
.userAgent("user agent")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont we expect products to set useragent as UserAgents.fromStrings("productName", "productVersion") ? Might be a good idea to document that or we will end up with just "productName"s as useragents.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

witchcraft will auto-gen the user agent for you, but yea I can update for general best practice.

@tpetracca tpetracca merged commit 628de22 into develop Oct 23, 2017
@tpetracca tpetracca deleted the require-user-agent branch October 23, 2017 18:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants