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

Rename "quorum" term to "split brain protection" #15444

Conversation

@petrpleshachkov
Copy link
Contributor

petrpleshachkov commented Aug 12, 2019

Introduced split brain protection concept to replace quorum one to make
it more explicit and unambiguous.

EE part: hazelcast/hazelcast-enterprise#3117
https://hazelcast.atlassian.net/wiki/spaces/PM/pages/346193957/Rename+Quorum+to+SplitBrain+Detection

Copy link
Contributor

blazember left a comment

Mostly LGTM.
There are a few things I think need to be fine-tuned after mass-renaming.
In general:

  • The important notes that meant to manage the expectations of the users regarding the term quorum I feel are unnecessary as we replaced the term.
  • I think using "presence" in conjuction with "split brain protection" is confusing. Mass-renaming produced many sentences in docs with "presence of split brain protection", "split brain protection presence", "split brain protection is present", "split brain protection should be present". These I think need to be rephrased.
specified one.
Adds the Split Brain Protection for this data-structure which you configure using the split-brain-protection element.
You should set the split-brain-protection-ref's value as the split brain protection's name.
IMPORTANT: The term "split brain protection" simply refers to the count of members in the cluster required for

This comment has been minimized.

Copy link
@blazember

blazember Aug 16, 2019

Contributor

I'm not sure this important note is still useful here. I think it was added only to manage the expectations of the users regarding the term quorum.

private boolean enabled;
private int size;
private List<SplitBrainProtectionListenerConfig> listenerConfigs = new ArrayList<SplitBrainProtectionListenerConfig>();
private SplitBrainProtectionType type = READ_WRITE;

This comment has been minimized.

Copy link
@blazember

blazember Aug 16, 2019

Contributor

I think it would be desired to align the programmatic configuration with the declarative configuration in terms of naming. Instead of type, protectOn should be used, size better to be replaced to minimumClusterSize, splitBrainProtectionFunctionClassName to functionClassName, same for implementation.


public SplitBrainProtectionConfig setSize(int size) {
if (size < 2) {
throw new InvalidConfigurationException("Minimum split brain protection size cannot be less than 2");

This comment has been minimized.

Copy link
@blazember

blazember Aug 16, 2019

Contributor

Minimum cluster size configured for split-brain protection cannot be less than 2?

* @see ProbabilisticSplitBrainProtectionFunction
*/
public static ProbabilisticSplitBrainProtectionConfigBuilder
newProbabilisticSplitBrainProtectionConfigBuilder(String name, int size) {

This comment has been minimized.

Copy link
@blazember

blazember Aug 16, 2019

Contributor

size -> minimumClusterSize here and in the recently active one too.

* split brain protection function, for the given split brain protection {@code size} that is enabled by default.
*
* @param name the split brain protection's name
* @param size minimum count of members for split brain protection to be considered present

This comment has been minimized.

Copy link
@blazember

blazember Aug 16, 2019

Contributor

Here mass-replacing made a joke of us :)
minimum count of members in the cluster not to be considered it split?

*
* @return boolean presence of the quorum
* @return boolean presence of the split brain protection
*/
boolean isPresent();

This comment has been minimized.

Copy link
@blazember

blazember Aug 16, 2019

Contributor

isMinimumClusterSizeSatisfied? Or something better :)
The docs in this file may need some adjustment too.

* It does NOT refer to an implementation of Paxos or Raft protocols as used in many NoSQL and distributed systems.
* The mechanism it provides in Hazelcast protects the user in case the number of nodes in a cluster drops below the
* specified one.
* IMPORTANT: The term "split brain protection" simply refers to the count of members in the cluster required for an

This comment has been minimized.

Copy link
@blazember

blazember Aug 16, 2019

Contributor

Same comment on the important notice than above.


import com.hazelcast.cluster.Member;
import com.hazelcast.cluster.MembershipListener;

import java.util.Collection;

/**
* A function that can be used to conclude absence/presence of quorum.
* The quorum function is consulted:<br>
* A function that can be used to conclude absence/presence of split brain protection.

This comment has been minimized.

Copy link
@blazember

blazember Aug 16, 2019

Contributor

"A function that can be used to conclude whether the minimum cluster size property is satisfied"?

Same for the apply method.

private final int heartbeatToleranceMillis;
private final ConcurrentMap<Member, Long> latestHeartbeatPerMember = new ConcurrentHashMap<Member, Long>();

public RecentlyActiveQuorumFunction(int quorumSize, int heartbeatToleranceMillis) {
this.quorumSize = quorumSize;
public RecentlyActiveSplitBrainProtectionFunction(int splitBrainProtectionSize, int heartbeatToleranceMillis) {

This comment has been minimized.

Copy link
@blazember

blazember Aug 16, 2019

Contributor

splitBrainProtectionSize -> minimumClusterSize


private enum SplitBrainProtectionState {
INITIAL,
PRESENT,

This comment has been minimized.

Copy link
@blazember

blazember Aug 16, 2019

Contributor

MIN_CLUSTER_SIZE_SATISFIED, MIN_CLUSTER_SIZE_NOT_SATISFIED?

@petrpleshachkov petrpleshachkov force-pushed the petrpleshachkov:feature/master/splitbrain-protection branch from 07b46cb to 17849ea Aug 28, 2019
@petrpleshachkov

This comment has been minimized.

Copy link
Contributor Author

petrpleshachkov commented Aug 30, 2019

run-lab-run

@petrpleshachkov

This comment has been minimized.

Copy link
Contributor Author

petrpleshachkov commented Aug 30, 2019

@blazember I've reviewed the comments and adjusted the code accordingly. Please let me know if you have more comments.

@petrpleshachkov petrpleshachkov force-pushed the petrpleshachkov:feature/master/splitbrain-protection branch from 8dfd858 to 36899c0 Sep 2, 2019
*/
public enum QuorumType {
public enum SplitBrainProtectionOn {

This comment has been minimized.

Copy link
@ahmetmircik

ahmetmircik Sep 2, 2019

Member

this may be called SplitBrainProtectionType.

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Sep 2, 2019

Author Contributor

In the configuration we have renamed type -> protectOn and this is why I've decided to rename it here as well. I know type might look better here but being consistent with the configuration seems more important.

Copy link
Member

ahmetmircik left a comment

LGTM

Introduced split brain protection concept to replace quorum one to make
it more explicit and unambiguous.
@petrpleshachkov petrpleshachkov dismissed stale reviews from vbekiaris, blazember, and ahmetmircik via 52f1d57 Sep 2, 2019
@petrpleshachkov petrpleshachkov force-pushed the petrpleshachkov:feature/master/splitbrain-protection branch from 36899c0 to 52f1d57 Sep 2, 2019
@petrpleshachkov

This comment has been minimized.

Copy link
Contributor Author

petrpleshachkov commented Sep 3, 2019

Guys, thank you very much for the review. I am merging the PR.

@petrpleshachkov petrpleshachkov merged commit a759e82 into hazelcast:master Sep 3, 2019
1 check passed
1 check passed
default Test PASSed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.