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

Do not serialize optimize-queries option in MapConfig #16524

Merged
merged 5 commits into from Jan 24, 2020

Conversation

cangencer
Copy link
Contributor

This setting is a derived value and when serializing there is no need to set it in the generated XML. Also fix the wrong conflict detection when both options are set.

This setting is a derived value and when serializing there
is no need to set it in the generated XML
@cangencer
Copy link
Contributor Author

run-lab-run

@vojtechtoman vojtechtoman self-requested a review January 22, 2020 14:08
@@ -401,6 +401,16 @@ public void givenSetOptimizeQueryIsTrue_whenSetCacheDeserializedValuesToINDEX_ON
mapConfig.setCacheDeserializedValues(CacheDeserializedValues.INDEX_ONLY);
}

@Test
public void givenSetOptimizeQueryIsFalse_whenSetCacheDeserializedValuesToNEVER_thenNoException() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not that important but this is just one of possible 4 allowed combinations. I don't think we cover setOptimizeQueries(false) followed by setCacheDeserializedValues(CacheDeserializedValues.INDEX_ONLY), and setCacheDeserializedValues(CacheDeserializedValues.NEVER/INDEX_ONLY) followed by setOptimizeQueries(false).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what's the expected behaviour when INDEX_ONLY is 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.

We actually had another bug which wasn't covered. I've updated the tests and cleaned up the code

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually don't know what the expected behaviour is with INDEX_ONLY, I simply looked at the current impl. INDEX_ONLY is the default and when setOptimizeQueries(false) is called, this default value does not change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, so nothing should happen when it's set to false and it's set to INDEX_ONLY?

Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, I actually don't know - maybe @jerrinot will be able to help?

@cangencer cangencer force-pushed the 3.x/optimize-queries branch 2 times, most recently from 371fd7b to 9778cb7 Compare January 22, 2020 17:12
@cangencer
Copy link
Contributor Author

cangencer commented Jan 23, 2020

I've changed it now that when optimizeQueries = false then cacheDeserializedValues can be INDEX_ONLY or NEVER. If it's true then the other setting must be ALWAYS

@cangencer
Copy link
Contributor Author

run-lab-run

@cangencer cangencer merged commit 5e0bdbc into hazelcast:maintenance-3.x Jan 24, 2020
@cangencer cangencer deleted the 3.x/optimize-queries branch January 24, 2020 10:41
@mmedenjak mmedenjak added the Source: Internal PR or issue was opened by an employee label Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants