Introduce NODE_AWARE partitioning group type #17889

hasancelik · 2020-11-24T13:20:30Z

For Kubernetes based environments, user might want to store their backups at another Kubernetes node. They can easily decide which pod will be running on which node via defining affinity or node-selector. To satisfy these kind of specific requirements, NODE_AWARE partition group type is introduced with this PR. Newly created NodeAwareMemberGroupFactory class is just simplified version of ZoneAwareMemberGroupFactory actually. Here is the related PRD.

These changes will be forward-port to 4.0.z, 4.1.z and master branches.

Todo:

Create related doc section at Partitioning Group Types --> added Node_Aware section into "Partition Group Configuration" section hazelcast-reference-manual#904

For Kubernetes based environments, user might want to store their backups at another Kubernetes node. They can easily decide which pod will be runing on which node via defining labels or node-selector. To satisfy these kind of specific requirements, NODE_AWARE partition group type is introduced. Newly created NodeAwareMemberGroupFactory class is just simplified version of ZoneAwareMemberGroupFactory.

leszko

Looks good. Added some (minor) comments, mainly about limiting this feature to Kubernetes. Other than that it looks, but please add also someone from the Core team to reviewers.

leszko · 2020-11-24T15:47:46Z

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

@@ -83,24 +83,34 @@
 * <p>
 * You can define as many <code>member-group</code>s as you want. Hazelcast will always store backups in a different
 * member-group to the primary partition.
+ *
+ * <h1>Zone Aware Partition Groups</h1>
+ * In this scheme, groups are allocated according to the metadata provided by Discovery SPI Partitions are not


groups are allocated according to the metadata provided by Discovery SPI Partitions are not written to the same group., could you re-write this sentence? Looks like some words are missing.

leszko · 2020-11-24T15:47:53Z

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

+ * <h1>Zone Aware Partition Groups</h1>
+ * In this scheme, groups are allocated according to the metadata provided by Discovery SPI Partitions are not
+ * written to the same group. This is very useful for ensuring partitions are written to availability
+ * zones or different racks without providing the IP addresses to the config ahead.


Actually, I did not write this part but as far as I can understand, rack is jclouds specific term and rack metadata is provided by jclouds and can be passed like this . Giving rack as a sample usage is not reasonable so I will remove it and use only zone.

leszko · 2020-11-24T15:48:45Z

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

 * In this scheme, groups are allocated according to the metadata provided by Discovery SPI Partitions are not
- * written to the same group. This is very useful for ensuring partitions are written to availability
- * zones or different racks without providing the IP addresses to the config ahead.
+ * written to the same group. This is very useful for ensuring partitions are written to different Kubernetes nodes


I don't think we should mention Kubernetes here? The mechanism is generic and it may be applied to some other orchestration solutions, like Docker Swarm.

Actually, I thought so at first, but I couldn't quite decide we should talk about other tools. When I think twice, you are right, we should mention other tools like Docker Swarm and ECS 👍

leszko · 2020-11-24T15:49:47Z

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

@@ -151,6 +161,11 @@
         * If only one zone is available, backups will be created in the same zone.
         */
        ZONE_AWARE,
+        /**
+         * Node Aware. Backups will be created in other Kubernetes nodes/physical machines.


The same, I don't think this feature is related only to Kubernetes.

leszko · 2020-11-24T15:51:43Z

hazelcast/src/main/java/com/hazelcast/partition/membergroup/NodeAwareMemberGroupFactory.java

+ * NodeAwareMemberGroupFactory is responsible for MemberGroups
+ * creation according to the Kubernetes Node metadata provided by
+ * {@link DiscoveryStrategy#discoverLocalMetadata()}
+ * @since 3.7


I think we can just remove this @since. If you want to keep it, please add the correct version.

copy paste error :) fixed 👍

leszko · 2020-11-24T15:53:01Z

hazelcast/src/main/java/com/hazelcast/partition/membergroup/NodeAwareMemberGroupFactory.java

+            final String nodeInfo = member.getStringAttribute(PartitionGroupMetaData.PARTITION_GROUP_NODE);
+            if (nodeInfo == null) {
+                throw new IllegalArgumentException("Not enough metadata information is provided. "
+                        + "Kubernetes node name information must be provided with NODE_AWARE partition group.");


The same, we'd better not limit it to Kubernetes

leszko · 2020-11-24T15:54:34Z

hazelcast/src/main/java/com/hazelcast/partition/membergroup/NodeAwareMemberGroupFactory.java

+            }
+            group.addMember(member);
+        }
+        return new HashSet<MemberGroup>(groups.values());


Suggested change

return new HashSet<MemberGroup>(groups.values());

return new HashSet<>(groups.values());

I have converted this line into diamond operator but PR builder fails with below error so I reverted it:

error: diamond operator is not supported in -source 1.6

Ahh, right, I forgot that it's a PR to 3.12.x and we need to support java 1.6. Then, it's fine. Leave it as it is.

leszko · 2020-11-24T15:55:48Z

hazelcast/src/main/java/com/hazelcast/spi/discovery/DiscoveryStrategy.java

-     * may include, but are not limited, to location information like datacenter, rack, host or additional
-     * tags to be used for custom purpose.
+     * may include, but are not limited, to location information like datacenter, rack, host,
+     * Kubernetes node name or additional tags to be used for custom purpose.


The same, I'd avoid limiting this feature to Kubernetes.

leszko · 2020-11-24T15:56:00Z

hazelcast/src/main/java/com/hazelcast/spi/discovery/integration/DiscoveryService.java

-     * may include, but are not limited, to location information like datacenter, rack, host or additional
-     * tags to be used for custom purpose.
+     * may include, but are not limited, to location information like datacenter, rack, host,
+     * Kubernetes node name or additional tags to be used for custom purpose.


The same, I'd avoid limiting this feature to Kubernetes

leszko · 2020-11-24T15:56:16Z

hazelcast/src/main/java/com/hazelcast/spi/partitiongroup/PartitionGroupMetaData.java

@@ -18,13 +18,17 @@

 /**
 * This class contains the definition of known Discovery SPI metadata to support automatic
- * generation of zone aware backup strategies based on cloud or service discovery provided
- * information. These information are split into three different levels of granularity:
+ * generation of zone aware and Kubernetes node aware backup strategies.


leszko · 2020-11-26T16:54:23Z

hazelcast/src/main/java/com/hazelcast/spi/partitiongroup/PartitionGroupMetaData.java

@@ -18,13 +18,19 @@

 /**
 * This class contains the definition of known Discovery SPI metadata to support automatic
- * generation of zone aware backup strategies based on cloud or service discovery provided
- * information. These information are split into three different levels of granularity:
+ * generation of zone aware and Kubernetes node aware backup strategies.


Suggested change

* generation of zone aware and Kubernetes node aware backup strategies.

* generation of zone aware and node aware backup strategies.

leszko · 2020-11-26T16:54:58Z

hazelcast/src/main/java/com/hazelcast/spi/partitiongroup/PartitionGroupStrategy.java

@@ -22,7 +22,7 @@
 /**
 * <p>A <tt>PartitionGroupStrategy</tt> implementation defines a strategy
 * how backup groups are designed. Backup groups are units containing
- * one or more Hazelcast nodes to share the same physical host, rack or
+ * one or more Hazelcast nodes to share the same physical host/Kubernetes node, rack or


Suggested change

* one or more Hazelcast nodes to share the same physical host/Kubernetes node, rack or

* one or more Hazelcast nodes to share the same physical host/node, rack or

leszko

Added a few super minor comments. Other than that, LGTM 👍

blazember

LGTM, with a few minors 👍

blazember · 2020-11-30T11:06:03Z

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

+ *
+ * <h1>Zone Aware Partition Groups</h1>
+ * In this scheme, groups are allocated according to the metadata provided by Discovery SPI
+ * These metadata are availability zone, rack and host. Partitions are not written to


Maybe something like "The backups of the partitions are not placed on..."?

blazember · 2020-11-30T11:59:16Z

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

+ * In this scheme, groups are allocated according to node name metadata provided by Discovery SPI.
+ * For container orchestration tools like Kubernetes and Docker Swarm, node is the term used to refer
+ * machine that containers/pods run on. A node may be a virtual or physical machine.
+ * Partitions are not written to the same group so this is very useful for ensuring partitions


blazember · 2020-11-30T12:04:20Z

hazelcast/src/main/java/com/hazelcast/partition/membergroup/NodeAwareMemberGroupFactory.java

+                groups.put(nodeInfo, group);
+            }
+            group.addMember(member);
+        }


I wonder if logging the group:member mappings at debug level would be useful.

For zone and node aware, I can say that user (should) already knows which member will be running on which node or zone via external deployment config. On the other hand, I agree with you it would be good beneficial for debug purposes. I have added into my todo list, I will prepare another PR for all group factories.

blazember · 2020-11-30T12:12:21Z

Don't forget about the documentation update (6.10.1) if there is no such PR in progress.

…enNoMetadataIsProvided

hasancelik added Type: Enhancement [OLD]Team: Integration Team: Core Source: Internal PR or issue was opened by an employee Module: Discovery SPI labels Nov 24, 2020

hasancelik added this to the 3.12.11 milestone Nov 24, 2020

hasancelik requested a review from leszko November 24, 2020 13:20

hasancelik mentioned this pull request Nov 24, 2020

Add NODE_AWARE metadata support hazelcast/hazelcast-kubernetes#278

Merged

1 task

leszko reviewed Nov 24, 2020

View reviewed changes

hasancelik added 2 commits November 25, 2020 11:24

fixed test failures

3413ad0

revised javadoc parts based on review comments

2c2bf32

hasancelik requested a review from leszko November 25, 2020 20:04

hasancelik added 2 commits November 25, 2020 23:42

use parameterized type

e46bda4

fixed checkstyle error

9a18d98

blazember self-requested a review November 26, 2020 11:02

leszko reviewed Nov 26, 2020

View reviewed changes

leszko approved these changes Nov 26, 2020

View reviewed changes

blazember approved these changes Nov 30, 2020

View reviewed changes

hasancelik added 3 commits November 30, 2020 17:05

review comments

fd6e116

removed since notation from NodeAwareMemberGroupFactory javadoc

a77634c

added testNodeAwareMemberGroupFactoryThrowsIllegalArgumentExceptionWh…

003b58c

…enNoMetadataIsProvided

hasancelik mentioned this pull request Dec 1, 2020

[FORWARD-PORT 4.1.z] Introduce NODE_AWARE partitioning group type #17912

Merged

hasancelik merged commit 242f5b9 into hazelcast:3.12.z Dec 1, 2020

This was referenced Dec 1, 2020

[FORWARD-PORT 4.0.z] Introduce NODE_AWARE partitioning group type #17913

Merged

[FORWARD-PORT] Introduce NODE_AWARE partitioning group type #17914

Merged

hazelcast deleted a comment Dec 21, 2020

hazelcast deleted a comment Dec 24, 2020

hasancelik mentioned this pull request May 10, 2021

Implementation of Partition Group Definition for ZookeeperDiscoveryStrategy hazelcast/hazelcast-zookeeper#92

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce NODE_AWARE partitioning group type #17889

Introduce NODE_AWARE partitioning group type #17889

hasancelik commented Nov 24, 2020 •

edited

leszko left a comment

leszko Nov 24, 2020

leszko Nov 24, 2020

hasancelik Nov 25, 2020

leszko Nov 24, 2020

hasancelik Nov 25, 2020

leszko Nov 24, 2020

leszko Nov 24, 2020

hasancelik Nov 25, 2020

leszko Nov 24, 2020

leszko Nov 24, 2020

hasancelik Nov 25, 2020

leszko Nov 26, 2020

leszko Nov 24, 2020

leszko Nov 24, 2020

leszko Nov 24, 2020

leszko Nov 26, 2020

leszko Nov 26, 2020

leszko left a comment

blazember left a comment

blazember Nov 30, 2020

blazember Nov 30, 2020

blazember Nov 30, 2020

hasancelik Nov 30, 2020

blazember commented Nov 30, 2020

	return new HashSet<MemberGroup>(groups.values());
	return new HashSet<>(groups.values());

	* generation of zone aware and Kubernetes node aware backup strategies.
	* generation of zone aware and node aware backup strategies.

	* one or more Hazelcast nodes to share the same physical host/Kubernetes node, rack or
	* one or more Hazelcast nodes to share the same physical host/node, rack or

Introduce NODE_AWARE partitioning group type #17889

Introduce NODE_AWARE partitioning group type #17889

Conversation

hasancelik commented Nov 24, 2020 • edited

leszko left a 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

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

Choose a reason for hiding this comment

leszko left a comment

Choose a reason for hiding this comment

blazember left a 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

blazember commented Nov 30, 2020

hasancelik commented Nov 24, 2020 •

edited