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

Fixing backward compatibility issue for PER_NODE capacity calculation algorithm #12195

Merged
merged 4 commits into from Apr 10, 2018

Conversation

Mak-Sym
Copy link

@Mak-Sym Mak-Sym commented Jan 25, 2018

Followup on discussion in this issue: #11646. This simple fix adds backward compatibility to data eviction algorithm for cache sizes, smaller than number of partitions. This can be extremely useful especially after introducing dynamic map configuration feature.

@Mak-Sym Mak-Sym changed the title Fixed backward compatibility issue for PER_NODE capacity calculation algorithm Fixing backward compatibility issue for PER_NODE capacity calculation algorithm Jan 25, 2018
@mmedenjak mmedenjak added this to the 3.10 milestone Jan 25, 2018
@mmedenjak
Copy link
Contributor

@Mak-Sym thanks for your contribution, before we can review & merge can you follow the instructions to sign and send the Hazelcast Contributor Agreement?

@Mak-Sym
Copy link
Author

Mak-Sym commented Jan 25, 2018

@mmedenjak I already did it.

@mmedenjak
Copy link
Contributor

Thanks, can you tell me when you sent it? I will try and find out why it got stuck.

@Mak-Sym
Copy link
Author

Mak-Sym commented Jan 25, 2018

Received: by 10.103.127.15 with HTTP; Wed, 3 Jan 2018 17:20:01 -0800 (PST)
Date: Thu, 4 Jan 2018 12:20:01 +1100
Delivered-To: maksym.fedoryshyn@gmail.com
Message-ID: <CAHD8j7ZM7gKKT8-2fFat1-2AYZUdEJk42KQiOYjJJ4OTzTSEMQ@mail.gmail.com>
Subject: Hazelcast Contributor Agreement
From: Maksym Fedoryshyn <maksym.fedoryshyn@gmail.com>
To: jaromir@hazelcast.com```

@jerrinot
Copy link
Contributor

hi,

I can confirm I received the agreement. I apologize for the delay.
Happy Hazelcasting!

@Mak-Sym
Copy link
Author

Mak-Sym commented Jan 25, 2018 via email

+ "Given the current cluster size of %d members with %d partitions, max size should be at "
+ "least %d.", mapConfig.getName(), memberCount, partitionCount, minMaxSize));
double perNodeMaxRecordStoreSize = (1D * configuredMaxSize * memberCount / partitionCount);
if(perNodeMaxRecordStoreSize < 1) {

Choose a reason for hiding this comment

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

Should compare against MIN_SANE_PER_PARTITION_SIZE here, rather than just 1, so that we never allow fewer records per node than the minimum.

Copy link
Contributor

Choose a reason for hiding this comment

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

This line will also fail with CheckStyle (we require a space here if ().

Please use CheckStyle on your local machine: mvn clean verify -Pcheckstyle -DskipTests

Copy link
Author

@Mak-Sym Mak-Sym Jan 30, 2018

Choose a reason for hiding this comment

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

@rjatkins I left ability to set up cache size smaller than MIN_SANE_PER_PARTITION_SIZE just in case clients want to do that for whatever reason. I tried to fix the case, when cache size doesn't make sense (in other words, when cache size is definitely wrong). At the same time, partition size of 1 may make sense under some conditions. But I can also create useRecomendedMinSettings flag, which will force set partition size to MIN_SANE_PER_PARTITION_SIZE (with another warn message probably). What do you think?

@Donnerbart thx for the feedback - code formatting is fixed now.

@mmedenjak mmedenjak added Source: Community PR or issue was opened by a community user Module: IMap labels Mar 27, 2018
@pveentjer
Copy link
Contributor

@rjatkins can you rebase?

@ahmetmircik
Copy link
Member

created a doc update issue for this PR: hazelcast/hazelcast-reference-manual#507

Copy link
Member

@ahmetmircik ahmetmircik left a comment

Choose a reason for hiding this comment

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

LGTM

@pveentjer pveentjer merged commit 6a2e584 into hazelcast:master Apr 10, 2018
@mmedenjak
Copy link
Contributor

5 new commits :( ah well...

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

7 participants