Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip namespace check for InMemoryKVS #2541

Merged
merged 2 commits into from Oct 26, 2017
Merged

Conversation

gsheasby
Copy link
Contributor

@gsheasby gsheasby commented Oct 23, 2017

Goals (and why):
We don't care about the namespace for InMemoryKVS, so we want to allow
any namespace to be set. Previously, each product that used InMemoryKVS
for their tests needed to configure the namespace to be exactly "test".

Implementation Description (bullets):

  • Set the default namespace to empty for InMemoryKVS
  • Ignore part of the config check when the KVS config is an InMemoryAtlasDbConfig

Concerns (what feedback would you like?): Is the config check ignoring too hacky?

Where should we start reviewing?: wherever, it's pretty small

Priority (whenever / two weeks / yesterday): this week, preferably this release as it's been hurting internal testing efforts.


This change is Reviewable

We don't care about the namespace for InMemoryKVS, so we want to allow
any namespace to be set. Previously, each product that used InMemoryKVS
for their tests needed to configure the namespace to be exactly "test".
@fsamuel-bs fsamuel-bs assigned fsamuel-bs and unassigned gmaretic Oct 24, 2017
@fsamuel-bs fsamuel-bs requested review from fsamuel-bs and removed request for gmaretic October 24, 2017 12:54
Copy link
Contributor

@fsamuel-bs fsamuel-bs left a comment

Choose a reason for hiding this comment

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

This change allows products to start without both a keyspace and a namespace config on InMemoryKVS. If none of the configs are specified we seem to fail on startup if the timelock block is present — https://github.com/palantir/atlasdb/blob/develop/atlasdb-config/src/main/java/com/palantir/atlasdb/factory/TransactionManagers.java#L588. We should probably make this a test case in TransactionManagersTest, as I don't know the behavior of timelock with client being empty string.

With this addition, I think this PR should be ready to merge.

@fsamuel-bs fsamuel-bs assigned gsheasby and unassigned fsamuel-bs Oct 24, 2017
@codecov-io
Copy link

codecov-io commented Oct 26, 2017

Codecov Report

Merging #2541 into develop will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #2541      +/-   ##
=============================================
+ Coverage      60.03%   60.07%   +0.03%     
+ Complexity      3389     2466     -923     
=============================================
  Files            860      859       -1     
  Lines          40119    40079      -40     
  Branches        4070     4073       +3     
=============================================
- Hits           24084    24076       -8     
+ Misses         14569    14540      -29     
+ Partials        1466     1463       -3
Impacted Files Coverage Δ Complexity Δ
...palantir/atlasdb/memory/InMemoryAtlasDbConfig.java 83.33% <100%> (ø) 5 <1> (+5) ⬆️
...ava/com/palantir/atlasdb/config/AtlasDbConfig.java 98% <100%> (-2%) 0 <0> (-24)
...antir/nexus/db/pool/HikariCPConnectionManager.java 52.75% <0%> (-3.15%) 14% <0%> (-2%)
...main/java/com/palantir/paxos/PaxosLearnerImpl.java 87.8% <0%> (-2.44%) 0% <0%> (ø)
.../com/palantir/atlasdb/table/common/TableTasks.java 57.69% <0%> (-0.35%) 20% <0%> (+20%)
...tlasdb/table/description/render/TableRenderer.java 68.41% <0%> (-0.21%) 0% <0%> (-9%)
...asdb/transaction/impl/AbstractTransactionTest.java 96.5% <0%> (-0.01%) 103% <0%> (+1%)
...alantir/atlasdb/memory/InMemoryAtlasDbFactory.java 21.21% <0%> (ø) 5% <0%> (+5%) ⬆️
...asdb/transaction/impl/SerializableTransaction.java 88.41% <0%> (ø) 86% <0%> (+86%) ⬆️
... and 27 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 59782d4...a8a0f30. Read the comment docs.

@gsheasby gsheasby assigned fsamuel-bs and unassigned gsheasby Oct 26, 2017
// Special case - empty timelock and empty namespace/keyspace does not make sense
boolean timelockClientNonEmpty = !timelock().get().client().orElse("").isEmpty();
Preconditions.checkState(timelockClientNonEmpty,
"For InMemoryKVS, the TimeLock client should not be empty");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could rewrite this as:

if (kvs is InMemory) {
    (namespacePresent && namespaceEqualsClientAndKeyspace) || timelockIsNotEmpty
} else {
    (namespacePresent && namespaceEqualsClientAndKeyspace) || (keyspacePresent && keyspaceEqualsClient)
}

Feel free to improve it even more. My goal here was to make it as descriptive as possible.

@fsamuel-bs
Copy link
Contributor

I'm going to approve it as is now, as we could refactor it later. The tests prove that we have the behavior we want here.

@fsamuel-bs fsamuel-bs merged commit 950c77f into develop Oct 26, 2017
@fsamuel-bs fsamuel-bs deleted the fix/in-memory-namespace branch October 26, 2017 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants