Implicitly enable MerkleTreeConfig for persistent IMap/ICache #19502

vbekiaris · 2021-09-09T14:14:24Z

When persistence is enabled, merkle tree maintenance has a
low memory overhead while improving the single member
crash-recovery use case. When MerkleTreeConfig#enabled
is not explicitly set to false by user, it is now implicitly
turned on for persistent IMap & ICache data structures.

Tests are in EE-side PR https://github.com/hazelcast/hazelcast-enterprise/pull/4283
Client protocol PR hazelcast/hazelcast-client-protocol#395

hz-devops-test · 2021-09-09T14:51:08Z

The job Hazelcast-pr-builder of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file

--------------------------
-------TEST FAILURE-------
--------------------------
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   ClientDynamicClusterConfigTest>DynamicConfigTest.testDefaultMapConfig:372->DynamicConfigTest.assertConfigurationsEqualOnAllMembers:742 expected: but was:
[ERROR]   ClientDynamicClusterConfigTest>DynamicConfigTest.testMapConfig_withEntryListenerClassName:390->DynamicConfigTest.assertConfigurationsEqualOnAllMembers:742 expected: but was:
[ERROR]   ClientDynamicClusterConfigTest>DynamicConfigTest.testMapConfig_withEntryListenerImplementation:381->DynamicConfigTest.assertConfigurationsEqualOnAllMembers:742 expected: but was:
[ERROR]   ClientDynamicClusterConfigTest>DynamicConfigTest.testMapConfig_withQueryCacheConfig:399->DynamicConfigTest.assertConfigurationsEqualOnAllMembers:742 expected: but was:
[ERROR]   ClientDynamicClusterConfigTest>DynamicConfigTest.testMapConfig_withQueryCacheConfig_andEntryListenerConfigByClassName:409->DynamicConfigTest.assertConfigurationsEqualOnAllMembers:742 expected: but was:
[ERROR]   ClientDynamicClusterConfigTest>DynamicConfigTest.testMapConfig_withQueryCacheConfig_andEntryListenerConfigByImplementation:419->DynamicConfigTest.assertConfigurationsEqualOnAllMembers:742 expected: but was:
[ERROR]   ConfigXmlGeneratorTest.testCacheAttributes:923 expected: but was:
[ERROR]   ConfigXmlGeneratorTest.testCacheFactoryAttributes:950 expected: but was:
[ERROR] Errors: 
[ERROR]   ClientDynamicConfigSmokeTest>DynamicConfigSmokeTest.map_testNonConflictingStaticConfig:242 ? InvalidConfiguration
[INFO] 
[ERROR] Tests run: 42811, Failures: 8, Errors: 1, Skipped: 1001
[INFO] 
[ERROR] There are test failures.

hazelcast/src/main/java/com/hazelcast/client/impl/clientside/ClientDynamicClusterConfig.java

vbekiaris · 2021-09-13T08:58:26Z

hazelcast/src/main/java/com/hazelcast/config/ConfigXmlGenerator.java

            appendEventJournalConfig(gen, m.getEventJournalConfig());
-            appendHotRestartConfig(gen, m.getHotRestartConfig());
+            appendDataPersistenceConfig(gen, m.getDataPersistenceConfig());


instead of deprecated hot-restart config, we should generate the current data-persistence config

Shouldn't we cover both configs to keep the generated configs equality? (Config1->XML->Config2)

Since we are merging HotRestartPersistenceConfig and PersistenceConfig during transformation from/to XML, a config with HotRestartPersistence -> XML (output as the new persistence element) -> parsed as config2 with PersistenceConfig will be equal to the source config. See also #19004 (comment). Is my reasoning correct @fbarotov ?

hmm, yeah we merge both HotRestartPersistenceConfig and PersistenceConfig when reading from XML, but when writing to XML we should write only HotRestartPersistenceConfig if the target can be HZ with version less than 5.0, otherwise just DataPersistenceConfig would do, since as Vassilis mentioned we merge two configs every time one changes.

On equality before/after transformation to/from XML, I just pushed a commit to fix it. I thought HotRestartPersistence-Persistence configs merge was already there but it was actually missing in ConfigXmlGenerator#generate method. Thanks for catching that @kwart .

On whether we should output hot-restart-persistence or persistence element: I suppose that starting with 5.0 we want people to migrate from the deprecated hot-restart-persistence configuration elements to the new persistence configuration. That's why I think we should output persistence in the generated XML from 5.0.
If we were to output both elements, then hot-restart-persistence would be a partial copy of the persistence element, since any new config will be only added to persistence config (eg see https://github.com/hazelcast/hazelcast/pull/19522/files#diff-ff0ffd59a12a51408de4fdac0b50ff4634ee86dad470c63d69a17359a815d16eR4049).
wdyt?

I discussed the issue with @fbarotov. In my opinion, we shouldn't proactively change the Config object. We should keep the config which user has configured. And the same config also store/generate to the XML. We should only resolve the proper persistence settings on places where we use it.

Faruk will work on a related issue hazelcast/hazelcast-enterprise#4281

Without doing a deep-dive on the consequences of this config merging, I'd say let's just keep it for now, unless there are severe correctness issues, rolling upgrade issues or something along those lines.

IMHO the conundrum of config equality when using ConfigXmlGenerator is not something we should dwell on for too long, especially this close to the release.

In other words, let's keep the current mechanism of proactively changing the config, until someone starts complaining. Maybe nobody will complain, and we'll remove the HR config in (hopefully) 5.1.

On whether we should output hot-restart-persistence or persistence element: I suppose that starting with 5.0 we want people to migrate from the deprecated hot-restart-persistence configuration elements to the new persistence configuration. That's why I think we should output persistence in the generated XML from 5.0.

Agree with this.

mdumandag · 2021-09-13T11:47:36Z

@vbekiaris Can you fetch the recent changes in the protocol repo and regenerate the binary compatibility files? We just merged a PR to that repo, so that's why the PR is showing conflicting files.

kwart

Some smaller comments were added. Otherwise LGTM.

hazelcast/src/main/java/com/hazelcast/internal/serialization/impl/SerializationUtil.java

hazelcast/src/main/java/com/hazelcast/config/ConfigXmlGenerator.java

kwart · 2021-09-13T13:26:30Z

hazelcast/src/main/java/com/hazelcast/config/ConfigXmlGenerator.java

            appendEventJournalConfig(gen, m.getEventJournalConfig());
-            appendHotRestartConfig(gen, m.getHotRestartConfig());
+            appendDataPersistenceConfig(gen, m.getDataPersistenceConfig());


Shouldn't we cover both configs to keep the generated configs equality? (Config1->XML->Config2)

hazelcast/src/main/java/com/hazelcast/config/ConfigXmlGenerator.java

vbekiaris · 2021-09-13T13:51:07Z

@vbekiaris Can you fetch the recent changes in the protocol repo and regenerate the binary compatibility files? We just merged a PR to that repo, so that's why the PR is showing conflicting files.

thanks for the nudge, done

mdumandag

Looks good from the client's perspective. I just lefted a question and a minor comment.

mdumandag · 2021-09-14T10:09:53Z

hazelcast/src/main/java/com/hazelcast/client/impl/clientside/ClientDynamicClusterConfig.java

@@ -134,6 +135,9 @@ public Config addMapConfig(MapConfig mapConfig) {
        Data partitioningStrategy = mapConfig.getPartitioningStrategyConfig() == null
                ? null : serializationService.toData(mapConfig.getPartitioningStrategyConfig().getPartitioningStrategy());

+        DataPersistenceAndHotRestartMerger
+                .merge(mapConfig.getHotRestartConfig(), mapConfig.getDataPersistenceConfig());


Question: Do we need to send the DataPersistenceConfig element in the DynamicConfigAddMapConfigCodec request below? I see that we only do it for HotRestartConfig. Or, merging the configuration elements and then just sending the HotRestartConfig is enough?

It is an issue, however unrelated to this PR. See also https://github.com/hazelcast/hazelcast-enterprise/issues/4282

hazelcast/src/test/java/com/hazelcast/config/ConfigXmlGeneratorTest.java

mdumandag · 2021-09-15T11:36:05Z

You need to update the compatibility binary files one more time after fetching the recent changes in the protocol and regenerating them, as another PR is just merged with the protocol changes

When persistence is enabled, merkle tree maintenance has a low memory overhead while improving the single member crash-recovery use case. When MerkleTreeConfig#enabled is not explicitly set to false by user, it is now implicitly turned on for persistent IMap & ICache data structures.

before generating XML to ensure equality of configs before/after transformation to/from XML

hz-devops-test · 2021-09-16T14:04:12Z

The job Hazelcast-pr-builder of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file

--------------------------
-------TEST FAILURE-------
--------------------------
[INFO] Results:
[INFO] 
[ERROR] Errors: 
[ERROR]   TestEmptyApplicationContext.testXmlWithoutConfig ? IllegalState Failed to load...
[INFO] 
[ERROR] Tests run: 201, Failures: 0, Errors: 1, Skipped: 0
[INFO] 
[ERROR] There are test failures.

vbekiaris · 2021-09-16T14:09:50Z

PR builder failed with #19548

vbekiaris added Type: Enhancement Team: Core Source: Internal PR or issue was opened by an employee Module: IMap Module: ICache Module: Hot Restart labels Sep 9, 2021

vbekiaris added this to the 5.0 milestone Sep 9, 2021

vbekiaris self-assigned this Sep 9, 2021

vbekiaris force-pushed the enhancements/5.0/persistence-config branch from da7bddb to dc00684 Compare September 13, 2021 08:02

vbekiaris marked this pull request as ready for review September 13, 2021 08:06

vbekiaris requested a review from a team as a code owner September 13, 2021 08:06

vbekiaris requested review from kwart and fbarotov September 13, 2021 08:06

vbekiaris commented Sep 13, 2021

View reviewed changes

hazelcast/src/main/java/com/hazelcast/client/impl/clientside/ClientDynamicClusterConfig.java Show resolved Hide resolved

vbekiaris commented Sep 13, 2021

View reviewed changes

vbekiaris mentioned this pull request Sep 13, 2021

Adds MerkleTreeConfig#enabledSet field hazelcast/hazelcast-client-protocol#395

Merged

kwart reviewed Sep 13, 2021

View reviewed changes

vbekiaris force-pushed the enhancements/5.0/persistence-config branch from a9915b4 to 28a8f7e Compare September 13, 2021 13:49

mdumandag reviewed Sep 14, 2021

View reviewed changes

fbarotov approved these changes Sep 15, 2021

View reviewed changes

vbekiaris requested a review from mdumandag September 15, 2021 10:58

mdumandag approved these changes Sep 15, 2021

View reviewed changes

mmedenjak requested a review from kwart September 16, 2021 06:15

kwart approved these changes Sep 16, 2021

View reviewed changes

vbekiaris added 2 commits September 16, 2021 16:26

Add tests for read/writeNullableBoolean

8aa6355

vbekiaris added 3 commits September 16, 2021 16:27

Merge hot-restart and persistence configs

01eeb05

before generating XML to ensure equality of configs before/after transformation to/from XML

Adds tests for MerkleTreeConfig equality

207e132

Updated client protocol compatibility files

a2bd13c

vbekiaris force-pushed the enhancements/5.0/persistence-config branch from 660561e to a2bd13c Compare September 16, 2021 13:28

mmedenjak merged commit 7fb387c into hazelcast:master Sep 16, 2021

hazelcast deleted a comment from hz-devops-test Sep 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implicitly enable MerkleTreeConfig for persistent IMap/ICache #19502

Implicitly enable MerkleTreeConfig for persistent IMap/ICache #19502

vbekiaris commented Sep 9, 2021 •

edited

hz-devops-test commented Sep 9, 2021

vbekiaris Sep 13, 2021

kwart Sep 13, 2021

vbekiaris Sep 13, 2021

fbarotov Sep 13, 2021

vbekiaris Sep 14, 2021

kwart Sep 14, 2021

kwart Sep 14, 2021

mmedenjak Sep 14, 2021

mmedenjak Sep 14, 2021

mdumandag commented Sep 13, 2021

kwart left a comment

kwart Sep 13, 2021

vbekiaris commented Sep 13, 2021

mdumandag left a comment

mdumandag Sep 14, 2021

vbekiaris Sep 14, 2021

mdumandag commented Sep 15, 2021

hz-devops-test commented Sep 16, 2021

vbekiaris commented Sep 16, 2021

Implicitly enable MerkleTreeConfig for persistent IMap/ICache #19502

Implicitly enable MerkleTreeConfig for persistent IMap/ICache #19502

Conversation

vbekiaris commented Sep 9, 2021 • edited

hz-devops-test commented Sep 9, 2021

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

Choose a reason for hiding this comment

mdumandag commented Sep 13, 2021

kwart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbekiaris commented Sep 13, 2021

mdumandag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdumandag commented Sep 15, 2021

hz-devops-test commented Sep 16, 2021

vbekiaris commented Sep 16, 2021

vbekiaris commented Sep 9, 2021 •

edited