ReplicatedMapProxy - refactored check and retry initialization logic ...

[netudima]

…to do it in parallel for different partitions. It is much faster compared to the original way - try to sync a partition and sleep.
Fixes #14330

[devOpsHazelcast]

All committers have signed the CLA.

[mmedenjak]

Hi @netudima ! After 1.5 years, we finally managed to take a look at the PR. To be honest, we haven't been good stewards of community PRs but things are changing around here.

I believe your PR makes perfect sense but there are a couple of small improvements I'd add here. First off, I think you can use BitSet instead of LinkedHashSet for the partition ID storage.

Secondly, since we're now adding support for many partitions (20k, 50k), I think we need to "throttle" loading a bit so maybe just add some local variable that will keep track of how many concurrent loading processes there are. For now, let's limit it to something like 100 - we can always change that number later.

And lastly, nitpicking - can you replace the while(true) with something like while (!nonLoadedStores.isEmpty()) to simplify the code?

If you're unable to continue with the PR since a lot of time has passed, we understand. I'll make sure we adopt the PR and continue with it and also mention you in the commit as the original author.

[netudima]

Hi @mmedenjak, thank you for the feedback :-). I will take a look and try to address your comments during 1-2 weeks.

[mmedenjak]

Looks like the issue is checkstyle:
10:51:01 [ERROR] /home/jenkins/jenkins_slave/workspace/Hazelcast-pr-builder_2/hazelcast/src/main/java/com/hazelcast/replicatedmap/impl/ReplicatedMapProxy.java:79:1: Class Fan-Out Complexity is 42 (max allowed is 40). [ClassFanOutComplexity]

If the issue persists after rebasing to latest master, I think you can ignore it, like here.

[netudima]

@mmedenjak, I have updated the PR based on the comments. Regarding while(true) - I have not found a way to simplify the logic without adding an unnecessary sleep for the case when the 1st iteration is successful for all partitions...

[mmedenjak]

Looks good, added one minor observation and one suggested change. Thank you for rebasing the fix!

[mmedenjak]

Minor: you can use nonLoadedStores.set(0, partitionCount); instead of looping.

[netudima]

I think even better option is possible :-) - I can rename the set to loadedStores and mark a bit as set when a partition loading is completed. In this case the init logic is not required.

[mmedenjak]

I tried doing something similar but beware in that the nextClearBit will never return -1 but might instead return a number which is even out of the partition ID range. So if all partitions are loaded, loadedPartitions.nextClearBit() will return partitionCount + 1

[tkountis]

@netudima thanks for your PR, much appreciated.
Do you mind including a test-case that demonstrates the fix, similar to the one you have in the linked issue.

[netudima]

It is a bit tricky, because the code from the issue shows a difference in a loading time, not in a functional behaviour. Maybe it can be tested using a test with timeout (but it can be unreliable when a test is running on a different hardware). Do you have some examples of unit tests with timeouts in Hazelcast codebase?

[tkountis]

@netudima I agree with you, i couldn't think of a solid test that wouldn't cause more pain to maintain.

[mmedenjak]

And after several years, we managed to get you to make a contribution. Thank you and here's a medal 🥇 cc @Holmistr

If you want, you can pick another issue. I can say right now everyone's a bit busy but we try to get community PRs merged faster these days.

[netudima]

🥳, thank you a lot for your help