Add NODE_AWARE metadata support #278

hasancelik · 2020-11-24T13:44:32Z

With 3.12.11 IMDG release, new metadata/group type introduced to define partition group based on Kubernetes node that Hazelcast cluster runs on. The discovery plugin should pass name of the node to the core SPI via using discoverLocalMetadata method like zone info. This method passes both zone and node name metadata. User can not enable two group types at the same time so cluster will decide which metadata will be used based on cluster config:

    hazelcast:
      partition-group:
        enabled: true
        group-type: NODE_AWARE

Here is the related PRD.

These changes will be forward-port to master(2.2.x) branch.

IMDG core counterpart PR .

Todo:

Add NODE_AWARE section into README

With 3.12.11 IMDG release, new metadata introduced to define partition group based on Kubernetes node that Hazelcast cluster runs on.The discovery plugin should pass name of the node to the core SPI via using discoverLocalMetadata method like zone info. This method passes both zone and node name metadata. User can not enable tow group types at the same time so cluster will decide which metadata will be used based on cluster config.

leszko

Added a few comments, but in general it looks good. Thanks for the PR @hasancelik

leszko · 2020-11-25T08:50:08Z

pom.xml

    <powermock.version>1.7.3</powermock.version>

-    <hazelcast.version>3.12.6</hazelcast.version>
+    <hazelcast.version>3.12.11-SNAPSHOT</hazelcast.version>


This change should be backward-compatible so that we wouldn't change compatibility in patch releases.

PartitionGroupMetaData.PARTITION_GROUP_NODE is/will be introduced with 3.12.11 so this version of hazelcast should be used, please check.

The other option is to not use PartitionGroupMetaData.PARTITION_GROUP_NODE in the kubernetes plugin, but just copy the same constant. It's just this one constant, so to be honest, I'd favor keeping backward-compatibility than reusing this one constant.

You are right, backward compatibility is more important than the convention for this scenario.

leszko · 2020-11-25T09:00:37Z

src/main/java/com/hazelcast/kubernetes/HazelcastKubernetesDiscoveryStrategy.java

+    private String discoverNodeName() {
+        if (DiscoveryMode.KUBERNETES_API.equals(config.getMode())) {
+            try {
+                String podName = System.getenv("POD_NAME");


These lines look the same as in discoverZone(). Could you extract it to a method String podName()?

leszko · 2020-11-25T09:01:18Z

src/main/java/com/hazelcast/kubernetes/HazelcastKubernetesDiscoveryStrategy.java

+                }
+                String nodeName = client.nodeName(podName);
+                if (nodeName != null) {
+                    getLogger().info(String.format("Kubernetes plugin extracted node name: %s", nodeName));


Suggested change

getLogger().info(String.format("Kubernetes plugin extracted node name: %s", nodeName));

getLogger().info(String.format("Kubernetes plugin discovered node name: %s", nodeName));

For the sake of consistency.

leszko · 2020-11-25T09:04:33Z

src/main/java/com/hazelcast/kubernetes/KubernetesClient.java

+     * @return Node name
+     * @see <a href="https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.11">Kubernetes Endpoint API</a>
+     */
+    String nodeName(String podName) {


Could you use this method in zone(String podName). Like the zone method should now look like this:

String nodeUrlString = String.format("%s/api/v1/nodes/%s", kubernetesMaster, nodeName(podName)); return extractZone(callGet(nodeUrlString));

hasancelik · 2020-11-25T10:17:56Z

Thanks for the review @leszko. I have addressed all of your comments, PTAL.

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

README.md

+ * Retrieving name of the node uses Kubernetes API, so RBAC must be configured as described [here](#granting-permissions-to-use-kubernetes-api)
+ * `NODE_AWARE` feature works correctly when Hazelcast members are distributed equally in all nodes, so your Kubernetes cluster must orchestrate PODs equally.
+
+ Note also that retrieving name of the node assumes that your container's hostname is the same as POD Name, which is almost always true. If you happen to change your hostname in the container, then please define the following environment variable:


Suggested change

Note also that retrieving name of the node assumes that your container's hostname is the same as POD Name, which is almost always true. If you happen to change your hostname in the container, then please define the following environment variable:

Note also that retrieving name of the node assumes that your container's hostname is the same as POD Name, which is almost always true. If you happen to change your hostname in the container, then please define the following environment variable:

leszko · 2020-11-26T16:59:32Z

README.md

        fieldPath: metadata.name
 ``` 

+### Node Aware


The content of the Node Aware section is almost the same as Zone Aware. Could you maybe merge both of them and create a section like Zone / Node Aware?

I know they are almost identical but separate section with own requirements and config is easily digestible. User wants to use Node Aware feature will not be disturbed by Zone Aware requirements or config.

Yeah, you're right. One thing that we could improve is to put them both under one section like "High Availability", "Zone/Node Failure Resilience", or "Zone/Node Aware". To have something like:

### High Availability #### Zone Aware #### Node Aware

Anyway, I leave it up to you.

leszko · 2020-11-26T17:02:25Z

pom.xml

    <powermock.version>1.7.3</powermock.version>

-    <hazelcast.version>3.12.6</hazelcast.version>
+    <hazelcast.version>3.12.11-SNAPSHOT</hazelcast.version>


The other option is to not use PartitionGroupMetaData.PARTITION_GROUP_NODE in the kubernetes plugin, but just copy the same constant. It's just this one constant, so to be honest, I'd favor keeping backward-compatibility than reusing this one constant.

leszko · 2020-11-26T17:03:34Z

src/main/java/com/hazelcast/kubernetes/HazelcastKubernetesDiscoveryStrategy.java

+                String podName = podName();
+                String nodeName = client.nodeName(podName);


Suggested change

String podName = podName();

String nodeName = client.nodeName(podName);

String nodeName = client.nodeName(podName());

Nit: Consider inlining.

leszko · 2020-12-01T05:28:21Z

README.md

        fieldPath: metadata.name
 ``` 

+### Node Aware


Yeah, you're right. One thing that we could improve is to put them both under one section like "High Availability", "Zone/Node Failure Resilience", or "Zone/Node Aware". To have something like:

### High Availability #### Zone Aware #### Node Aware

Anyway, I leave it up to you.

* Add NODE_AWARE metadata support With 3.12.11 IMDG release, new metadata introduced to define partition group based on Kubernetes node that Hazelcast cluster runs on.The discovery plugin should pass name of the node to the core SPI via using discoverLocalMetadata method like zone info. This method passes both zone and node name metadata. User can not enable tow group types at the same time so cluster will decide which metadata will be used based on cluster config.

hasancelik added the enhancement label Nov 24, 2020

hasancelik added this to the 1.5.5 milestone Nov 24, 2020

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

leszko reviewed Nov 25, 2020

View reviewed changes

adressed review comments

00d3067

hasancelik requested a review from leszko November 25, 2020 10:19

added NODE_AWARE section into README

b78da86

leszko reviewed Nov 26, 2020

View reviewed changes

review comments

22b3a08

hasancelik requested a review from leszko November 30, 2020 13:22

leszko approved these changes Dec 1, 2020

View reviewed changes

added High Availability section into README

72e3eec

hasancelik merged commit 9f70794 into hazelcast:1.5.x Dec 1, 2020

hasancelik mentioned this pull request Dec 1, 2020

[FORWARD-PORT] Add NODE_AWARE metadata support #281

Merged

	getLogger().info(String.format("Kubernetes plugin extracted node name: %s", nodeName));
	getLogger().info(String.format("Kubernetes plugin discovered node name: %s", nodeName));

	Note also that retrieving name of the node assumes that your container's hostname is the same as POD Name, which is almost always true. If you happen to change your hostname in the container, then please define the following environment variable:
	Note also that retrieving name of the node assumes that your container's hostname is the same as POD Name, which is almost always true. If you happen to change your hostname in the container, then please define the following environment variable:

		String podName = podName();
		String nodeName = client.nodeName(podName);

Add NODE_AWARE metadata support #278

Add NODE_AWARE metadata support #278

Uh oh!

Conversation

hasancelik commented Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leszko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasancelik commented Nov 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hasancelik commented Nov 24, 2020 •

edited

Loading