Add http server port in the worker net address #18499

ChunxuTang · 2024-01-25T21:54:43Z

What changes are proposed in this pull request?

This PR enables registering the worker's HTTP server's port in the etcd. This helps to find worker's restful APIs from the Python client.

Why are the changes needed?

Alluxio Python client (e.g. in ML use cases) needs to connect to the worker's REST APIs. But as the http server port isn't included in the worker's information in the etcd, the client fails to find the API endpoint.

Does this PR introduce any user facing changes?

No.

ChunxuTang · 2024-01-27T00:36:46Z

Hi @lucyge2022 @jja725 @dbw9580 @ssyssy , could you take a review of this etcd registration change?

ChunxuTang · 2024-01-27T00:36:52Z

Verified the backward compatibility by
start worker with old version -> stop worker -> start new version of worker -> see if it works
Cc @lucyge2022

dbw9580 · 2024-01-29T06:41:07Z

dora/core/common/src/main/java/alluxio/wire/WorkerNetAddress.java

+  @com.google.gson.annotations.SerializedName("HttpServerPort")
+  private int mHttpServerPort;


This port is for the restful API server on the worker, right? There is already a "web port" which runs the web UI. Since both the UI and the restful API runs over HTTP, and possibly more services that run over HTTP will be added, can we be more specific that this port is used for the restful API exclusively?

Suggested change

@com.google.gson.annotations.SerializedName("HttpServerPort")

private int mHttpServerPort;

@com.google.gson.annotations.SerializedName("RestApiPort")

private int mRestApiPort;

The HttpServerPort is used in the PropertyKey as the config for the restful API. The name is WORKER_HTTP_SERVER_PORT. If we change the mHttpServerPort to mRestApiPort, it will cause naming inconsistencies since other WorkerNetAddress variables (e.g. mWebPort, mDataPort) follow the same names as configs in the PropertyKey.

I suggest keeping the name for now. If we want to change the name in the future, we'll need to change both this one and the property key (and probably any other places using HTTP server port...).

can u also add some comment here, saying this is a "optional"-like field which will not be included in equalness comparison

dbw9580 · 2024-01-29T06:42:23Z

dora/core/common/src/main/java/alluxio/wire/WorkerNetAddress.java

+        // Skip the comparison of mHttpServerPort for backward compatibility
+        // && mHttpServerPort == that.mHttpServerPort;


what's incompatible with comparing the restful api port for equality?

also update the hashCode implementation, or leave a comment explaining why mHttpServerPort is excluded from computing the hashcode.

Incompatibility.

As @lucyge2022 mentioned, the previous etcd-registered worker information doesn't have HTTP server ports. 1) When we do the comparison, the lack of HTTP ports will cause an issue. 2) It will also influence worker registration if we just add an HTTP port for an old worker (I added a new test to cover this scenario).

hashCode.

Updated with a comment on the exclusion of mHttpServerPort.

When we do the comparison, the lack of HTTP ports will cause an issue

When checking equality, WorkerServiceEntity explicitly ignores all ephemeral information about a worker such as network addresses, and only compares the worker identity. This is why WorkerIdentity exists. Therefore, a different HTTP port won't cause issue, as any other existing network addresses like hostname and grpc port.

lucyge2022 · 2024-01-29T21:23:27Z

dora/core/common/src/main/java/alluxio/membership/EtcdMembershipManager.java

-                  + "clean local worker identity settings to continue.",
-                  existingEntity.get().getWorkerNetAddress().toString(),
-                  existingEntity.get().getIdentity()));
+          if (existingEntity.get().equals(entity)) {


add some comment below, like
// same entity, update with new information.

lucyge2022 · 2024-01-29T21:27:00Z

dora/tests/testcontainers/src/test/java/alluxio/membership/MembershipManagerTest.java

    WorkerInfo wkr2 = new WorkerInfo()
        .setIdentity(workerIdentity1)
        .setAddress(new WorkerNetAddress()
            .setHost("worker2").setContainerHost("containerhostname2")
            .setRpcPort(2000).setDataPort(2001).setWebPort(2011)
-            .setDomainSocketPath("/var/lib/domain.sock"));
+            .setDomainSocketPath("/var/lib/domain.sock").setHttpServerPort(2021));
    membershipManager.join(wkr1);
    // bring wrk1 down and join wrk2 with a same worker identity.
    membershipManager.stopHeartBeat(wkr1);


can u add a test to have a worker join without http port set and afterwards join with http port set , it should be allowed to join and the list of workers should be unchanged.

Added a test testWorkerHttpServerPorts. Could you take a review?

lucyge2022 · 2024-01-29T21:28:14Z

dora/core/common/src/main/java/alluxio/wire/WorkerNetAddress.java

@@ -233,6 +254,8 @@ public boolean equals(Object o) {
        && mDataPort == that.mDataPort
        && mWebPort == that.mWebPort
        && mDomainSocketPath.equals(that.mDomainSocketPath);
+        // Skip the comparison of mHttpServerPort for backward compatibility
+        // && mHttpServerPort == that.mHttpServerPort;


can u remove line 258?

lucyge2022 · 2024-01-29T22:10:03Z

Verified the backward compatibility by start worker with old version -> stop worker -> start new version of worker -> see if it works Cc @lucyge2022

cool perfect thanks!

dbw9580 · 2024-01-30T09:11:30Z

dora/core/common/src/main/java/alluxio/membership/WorkerServiceEntity.java

    // only the service name and the identity are the factors that define
    // what a WorkerServiceEntity is. Any other is either ephemeral or supplementary data.
    return mIdentity.equals(anotherO.mIdentity)
-        && getServiceEntityName().equals(anotherO.getServiceEntityName());
+        && getServiceEntityName().equals(anotherO.getServiceEntityName())
+        && mAddress.equals(anotherO.getWorkerNetAddress());


The comment explicitly states that WorkerServiceEntity ignores worker address for equality comparison. If a worker with the same ID restarts and the address changes, it will leave two different entries on etcd and causes the clients to think there are multiple workers, but there are actually only one worker.

Bowen, actually when collision happens, in etcd comparison it directly compares the value bytes (EtcdMembershipManager.java line 134) and under that situation we evaluate if there's a need to update, and then we use equals here, if equals we know its the same worker and then update with new info, this is to introduce backward compatibility after adding a field "httpport" into the info to persist on etcd. Can u check if this equals is used somewhere else apart from etcdmembership join that will cause other problems?

dbw9580 · 2024-01-30T09:16:21Z

dora/core/common/src/main/java/alluxio/wire/WorkerNetAddress.java

+        // Skip the comparison of mHttpServerPort for backward compatibility
+        // && mHttpServerPort == that.mHttpServerPort;


When we do the comparison, the lack of HTTP ports will cause an issue

When checking equality, WorkerServiceEntity explicitly ignores all ephemeral information about a worker such as network addresses, and only compares the worker identity. This is why WorkerIdentity exists. Therefore, a different HTTP port won't cause issue, as any other existing network addresses like hostname and grpc port.

lucyge2022 · 2024-01-30T19:29:19Z

dora/tests/testcontainers/src/test/java/alluxio/membership/MembershipManagerTest.java

+  public void testWorkerHttpServerPorts() throws Exception {
+    final MembershipManager membershipManager = getHealthyEtcdMemberMgr();
+    Configuration.set(PropertyKey.WORKER_MEMBERSHIP_MANAGER_TYPE, MembershipType.ETCD);
+    Configuration.set(PropertyKey.ETCD_ENDPOINTS, getClientEndpoints());


u dont need to set the config here anymore if getHealthyEtcdMemberMgr is used

lucyge2022 · 2024-01-30T19:30:46Z

dora/tests/testcontainers/src/test/java/alluxio/membership/MembershipManagerTest.java

+    workerNetAddress.setHttpServerPort(1021);
+    membershipManager.join(wkr);
+    // check if the worker is rejoined and information updated
+    curWorkerInfo = membershipManager.getLiveMembers().getWorkerById(workerIdentity);


can you also assert the getallmembers only have one worker -> meaning that we are not creating two entries

That's a good point! I added that assertion.

lucyge2022 · 2024-01-30T19:32:25Z

dora/tests/testcontainers/src/test/java/alluxio/membership/MembershipManagerTest.java

@@ -401,4 +401,44 @@ public void testSameWorkerIdentityConflict() throws Exception {
    Assert.assertTrue(curWorkerInfo.isPresent());
    Assert.assertEquals(wkr2.getAddress(), curWorkerInfo.get().getAddress());
  }
+
+  @Test
+  public void testWorkerHttpServerPorts() throws Exception {


can u say testOptionalFieldChangeInWorkerAddress

Done. Updated the function name to testOptionalHttpPortChangeInWorkerAddress.

ChunxuTang · 2024-01-31T04:24:19Z

@lucyge2022 @dbw9580 I updated the PR according to our discussion. Could you take a look again?

dbw9580 · 2024-01-31T05:14:09Z

dora/core/common/src/main/java/alluxio/wire/WorkerNetAddress.java

+  public boolean customizedEquals(Object o) {
+    if (this == o) {
+      return true;
+    }
+    if (!(o instanceof WorkerNetAddress)) {
+      return false;
+    }
+    WorkerNetAddress that = (WorkerNetAddress) o;
+    return mHost.equals(that.mHost)
+        && mContainerHost.equals(that.mContainerHost)
+        && mSecureRpcPort == that.mSecureRpcPort
+        && mRpcPort == that.mRpcPort
+        && mDataPort == that.mDataPort
+        && mWebPort == that.mWebPort
+        && mDomainSocketPath.equals(that.mDomainSocketPath);


I think this does not have to be put in WorkerNetAddress because the costumized comparison can be different depending on the caller. You can remove this and move code to WorkerServiceEntity.

Sure. I moved the comparison logic to WorkerServiceEntity.

dbw9580 · 2024-01-31T05:14:28Z

dora/core/common/src/main/java/alluxio/membership/WorkerServiceEntity.java

+    WorkerServiceEntity anotherO = (WorkerServiceEntity) o;
+    return mIdentity.equals(anotherO.mIdentity)
+        && getServiceEntityName().equals(anotherO.getServiceEntityName())
+        && mAddress.customizedEquals(anotherO.getWorkerNetAddress());


move the comparison here

use name as consideredEquivalent ?

yeah, or be explicit about what is considered equivalent, e.g. equalsIgnoringOptionalFields

Updated the function name to equalsIgnoringOptionalFields.

dbw9580

LGTM, thanks!

ChunxuTang · 2024-01-31T19:38:51Z

@lucyge2022 @dbw9580 @ssyssy @jja725 Thanks for your review!

ChunxuTang · 2024-01-31T19:40:14Z

alluxio-bot, merge this please

alluxio-bot · 2024-01-31T19:40:16Z

merge failed:
Merge refused because pull request does not have label start with type-

ChunxuTang · 2024-01-31T19:40:51Z

alluxio-bot, merge this please

ChunxuTang added 2 commits January 25, 2024 13:53

Add http server port in the worker net address

f1ef54d

Overwrite etcd worker registration without http ports

cb0ab35

ChunxuTang changed the title ~~[WIP- Don't review] Add http server port in the worker net address~~ Add http server port in the worker net address Jan 27, 2024

ChunxuTang requested review from dbw9580, lucyge2022, jja725 and ssyssy January 27, 2024 00:35

ssyssy approved these changes Jan 27, 2024

View reviewed changes

dbw9580 reviewed Jan 29, 2024

View reviewed changes

lucyge2022 reviewed Jan 29, 2024

View reviewed changes

Resolve review comments

77f67a3

dbw9580 suggested changes Jan 30, 2024

View reviewed changes

lucyge2022 reviewed Jan 30, 2024

View reviewed changes

ChunxuTang added 2 commits January 30, 2024 17:17

Fix review comment

07c139e

Use customized equality to compare etcd worker identifiers

ff94a20

dbw9580 reviewed Jan 31, 2024

View reviewed changes

Move customized equality comparison to WorkerServiceEntity

da942d7

dbw9580 approved these changes Jan 31, 2024

View reviewed changes

lucyge2022 approved these changes Jan 31, 2024

View reviewed changes

ChunxuTang added the area-s3-rest-api label Jan 31, 2024

ChunxuTang added area-worker Alluxio worker workload-machine-learning Machine Learning Workloads labels Jan 31, 2024

ChunxuTang added the type-feature This issue is a feature request label Jan 31, 2024

alluxio-bot merged commit 262b2b3 into Alluxio:main Jan 31, 2024
14 checks passed

ChunxuTang deleted the http-port branch January 31, 2024 19:41

ChunxuTang mentioned this pull request Jan 31, 2024

Read worker http ports from etcd Alluxio/alluxio-py#53

Merged

		@com.google.gson.annotations.SerializedName("HttpServerPort")
		private int mHttpServerPort;

		// Skip the comparison of mHttpServerPort for backward compatibility
		// && mHttpServerPort == that.mHttpServerPort;

Add http server port in the worker net address #18499

Add http server port in the worker net address #18499

Conversation

ChunxuTang commented Jan 25, 2024 • edited Loading

What changes are proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user facing changes?

ChunxuTang commented Jan 27, 2024

ChunxuTang commented Jan 27, 2024

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

ChunxuTang Jan 30, 2024 • edited Loading

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

lucyge2022 commented Jan 29, 2024

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

ChunxuTang commented Jan 31, 2024

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

dbw9580 left a comment

Choose a reason for hiding this comment

ChunxuTang commented Jan 31, 2024

ChunxuTang commented Jan 31, 2024

alluxio-bot commented Jan 31, 2024

ChunxuTang commented Jan 31, 2024

ChunxuTang commented Jan 25, 2024 •

edited

Loading

ChunxuTang Jan 30, 2024 •

edited

Loading