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

Live Reloading the TimeLock Block, Part 4: Documentation and Release Notes #2648

Merged
merged 7 commits into from
Nov 14, 2017

Conversation

jeremyk-91
Copy link
Contributor

@jeremyk-91 jeremyk-91 commented Nov 7, 2017

Goals (and why):
Live reload the timelock block - see #2621, #2622 and #2647
This is the last in the series for now. There is a Part 5 I might want to do (ETE testing) but that's not as high on internal priority list at this time.

Implementation Description (bullets):

  • Document how the live reloadable config works
  • Document some of the pitfalls involved, and semantics for some edge cases.

Concerns (what feedback would you like?):

  • Do the docs make sense?
  • The release notes are quite long, mainly because this change does affect quite a bunch of things.

Where should we start reviewing?: timelock_client_config.rst

Priority (whenever / two weeks / yesterday): this week?


This change is Reviewable

@jeremyk-91 jeremyk-91 requested review from gmaretic and tboam November 7, 2017 20:49
Copy link
Contributor

@tboam tboam left a comment

Choose a reason for hiding this comment

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

Looks good, few suggestions for things that would be cleaner.

.. warning::

Although we support starting up without knowledge of any TimeLock nodes, note that if you are using TimeLock
your service will fail to start if asynchronous initialization (``initializeAsync``) is set to ``false``, as
Copy link
Contributor

Choose a reason for hiding this comment

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

"...will fail to start if there are no TimeLock nodes and asynchronous initialization..." is clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, changed.


Optional parameters:
* - serversList::sslConfiguration
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this change with the move to live reloading? Can you have live reloaded list of Timelock nodes but a static security block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - but the static security block must be provided in the runtime config. I've added another paragraph to the note here explaining how this is handled.


Also, although we support live-reloading of the server configuration, AtlasDB needs to know at install time that it
should talk to TimeLock - thus, the install configuration must contain a ``timelock`` block (even if said block is
possibly empty).
Copy link
Contributor

Choose a reason for hiding this comment

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

How do I specify an empty block? Does it contain nothing or does it need something to say it is a object with no fields set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added an example of an empty block (it is the YAML empty object: {})

@jeremyk-91 jeremyk-91 force-pushed the jkong/lrt-3 branch 2 times, most recently from bb68dd4 to 2f5debf Compare November 10, 2017 22:43
@jeremyk-91 jeremyk-91 self-assigned this Nov 14, 2017
@codecov-io
Copy link

codecov-io commented Nov 14, 2017

Codecov Report

Merging #2648 into jkong/lrt-3 will decrease coverage by 0.31%.
The diff coverage is 80.41%.

Impacted file tree graph

@@                Coverage Diff                @@
##             jkong/lrt-3    #2648      +/-   ##
=================================================
- Coverage          60.36%   60.04%   -0.32%     
- Complexity          4467     4470       +3     
=================================================
  Files                871      868       -3     
  Lines              40105    40425     +320     
  Branches            4021     4054      +33     
=================================================
+ Hits               24208    24272      +64     
- Misses             14420    14678     +258     
+ Partials            1477     1475       -2
Impacted Files Coverage Δ Complexity Δ
...palantir/atlasdb/config/TimeLockClientConfigs.java 66.66% <ø> (ø) 0 <0> (ø) ⬇️
.../palantir/atlasdb/config/AtlasDbRuntimeConfig.java 100% <ø> (ø) 6 <0> (ø) ⬇️
...lantir/atlasdb/http/AtlasDbFeignTargetFactory.java 0% <0%> (ø) 0 <0> (ø) ⬇️
.../com/palantir/atlasdb/config/ServerListConfig.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...ava/com/palantir/atlasdb/config/AtlasDbConfig.java 98.18% <100%> (+2.86%) 26 <2> (-1) ⬇️
...a/com/palantir/atlasdb/factory/ServiceCreator.java 85% <100%> (ø) 1 <0> (ø) ⬇️
...palantir/atlasdb/config/TimeLockRuntimeConfig.java 100% <100%> (ø) 0 <0> (ø) ⬇️
...ntir/atlasdb/factory/startup/TimeLockMigrator.java 100% <100%> (ø) 0 <0> (ø) ⬇️
.../com/palantir/atlasdb/http/AtlasDbHttpClients.java 69.23% <100%> (-3.19%) 4 <0> (ø)
.../palantir/atlasdb/config/TimeLockClientConfig.java 75% <50%> (-8.34%) 1 <0> (-1)
... and 69 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 030539f...fe7d1ed. Read the comment docs.

@jeremyk-91 jeremyk-91 changed the base branch from jkong/lrt-3 to develop November 14, 2017 20:43
@jeremyk-91 jeremyk-91 merged commit d1d30df into develop Nov 14, 2017
@jeremyk-91 jeremyk-91 deleted the jkong/lrt-4 branch November 14, 2017 20:46
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