Update worker hash ring #58

LuQQiu · 2024-02-07T02:24:24Z

Update worker hash ring to use worker identity, align with AOS 309

LuQQiu · 2024-02-07T02:25:13Z

Add and create the test file via adding following code to dora

ConsistentHashProviderTest
  @Test
  public void sourceOfTruth() throws IOException {
    Gson gson = new GsonBuilder().setPrettyPrinting().create();
    ConsistentHashProvider provider = new ConsistentHashProvider(
        100, 100000000, 5);
    Set<WorkerIdentity> workerList = generateRandomWorkerList(50);
    // set initial state
    provider.refresh(workerList);
    NavigableMap<Integer, WorkerIdentity> map = provider.getActiveNodesMap();

    try (FileWriter writer = new FileWriter("/Users/alluxio/downloads/testData/workerList.json")) {
      gson.toJson(workerList, writer);
    }

    // Serialize and write active nodes map to a file
    try (FileWriter writer = new FileWriter("/Users/alluxio/downloads/testData/activeNodesMap.json")) {
      gson.toJson(map, writer);
    }

    List<String> ufsUrls = Arrays.asList(
        "s3://bucket/path/to/dir",
        "s3://bucket/path/to/file",
        "s://ai-testing/",
        "hdfs://host:port/path/to/dir",
        "hdfs://host:port/path/to/file",
        "s3://ai-ref-arch/yelp-review/model.pt",
        "s3://ai-ref-arch/yelp-review/yelp_academic_dataset_business.json",
        "s3://ai-ref-arch/yelp-review/yelp_academic_dataset_checkin.json",
        "s3://ai-ref-arch/yelp-review/yelp_academic_dataset_review.json",
        "s3://ai-ref-arch/yelp-review/yelp_academic_dataset_tip.json",
        "s3://ai-ref-arch/yelp-review/yelp_academic_dataset_user.json",
        "s3://ai-ref-arch/yelp-review/yelp_review_sample.csv",
        "s3://ai-ref-arch/yelp-review/yelp_review_sample_large.csv",
        "hdfs://namenode:8020/user/hadoop/dir",
        "hdfs://namenode:8020/user/hadoop/file.txt",
        "wasbs://container@account.blob.core.windows.net/dir",
        "wasbs://container@account.blob.core.windows.net/file.txt",
        "gs://bucket/dir",
        "gs://bucket/file.txt",
        "gcs://bucket/dir",
        "gcs://bucket/file.txt"
    );

    try (FileWriter writer = new FileWriter("/Users/alluxio/downloads/testData/fileUrlWorkers.json")) {
      Map<String, List<WorkerIdentity>> fileUrlWorkers = new HashMap<>();
      for (String ufsUrl : ufsUrls) {
        List<WorkerIdentity> selectedWorkers = provider.getMultiple(ufsUrl, 5);
        
        fileUrlWorkers.put(ufsUrl, selectedWorkers);
      }
      gson.toJson(fileUrlWorkers, writer);
    }
  }

@ChunxuTang @jja725 Cannot get the hash ring result correct so far

ChunxuTang

Generally looks good to me. Just left some minor comments.

BTW, I saw there are quite a number of commits in this PR. Could you squash them into fewer comments?

ChunxuTang · 2024-02-16T21:54:16Z

alluxio/alluxio_file_system.py

-            worker_hosts (str, optional):
-                The worker hostnames in host1,host2,host3 format. Either etcd_hosts or worker_hosts should be provided, not both.


I would suggest keeping the worker_hosts for compatibility, although the functionality is probably broken at this moment. Instead, we can add a warning when users only provide workers without etcd host(s).

This is a change of the AlluxioFileSystem, the interface used by other applications. If we remove the variable (backward compatibility breaks), we have to update the fsspec. Afterward, when we add worker hosts back, we'll have to update the fsspec again (break again).

Make sense, i will leave it here and fix it in the later static membership PR

ChunxuTang · 2024-02-16T21:55:01Z

alluxio/alluxio_file_system.py

-        if etcd_hosts is None and worker_hosts is None:
-            raise ValueError(
-                "Must supply either 'etcd_hosts' or 'worker_hosts'"
-            )
-        if etcd_hosts and worker_hosts:
-            raise ValueError(
-                "Supply either 'etcd_hosts' or 'worker_hosts', not both"
-            )


Suggest changing it to a warning if users only provide worker hosts without etcd hosts.

ChunxuTang · 2024-02-16T21:59:45Z

alluxio/worker_ring.py

-        self.host = host
-        self.port = port
+        self._host = host
+        self._port = port


After reviewing other parts of the code, I would say we generally use names without the leading underscore for data members. Changing them back to self.host and self.port?

the google python convention is using underscore to indicate private member values. I remember some other parts have changed to this new convention

How about i create another PR to fix other parts of the code base after this PR

Sure! Thanks.

ChunxuTang · 2024-02-16T21:59:59Z

alluxio/worker_ring.py

-        self.etcd_username = None
-        self.etcd_password = None
-        self.prefix = ETCD_PREFIX_FORMAT.format(
+        self._etcd_username = None
+        self._etcd_password = None
+        self._prefix = ETCD_PREFIX_FORMAT.format(


Same as above.

ChunxuTang · 2024-02-16T22:03:46Z

alluxio/worker_ring.py

+            worker_addresses = []
+            for worker_identity in worker_identities:
+                worker_address = self._worker_info_map.get(worker_identity)
+                if (


Unnecessary to have the parenthesis around the worker_address?

ChunxuTang · 2024-02-16T22:04:55Z

alluxio/worker_ring.py

-        if worker_addresses != self._worker_addresses:
-            self._update_hash_ring(worker_addresses)
+        worker_info_map = {}
+        detect_diff_in_worker_info = False


=> diff_in_worker_info_deteced

jja725

Thanks for the improvement! Make some comments

jja725 · 2024-02-20T21:50:19Z

alluxio/alluxio_file_system.py

        self.logger = logger or logging.getLogger("AlluxioFileSystem")
+        if etcd_hosts and not worker_hosts:


We can use Union type in python to indicate OR semantic

jja725 · 2024-02-20T21:51:46Z

alluxio/alluxio_file_system.py

        self.session = self._create_session(concurrency)

        # parse options
        page_size = ALLUXIO_PAGE_SIZE_DEFAULT_VALUE
+        hash_node_per_worker = int(ALLUXIO_HASH_NODE_PER_WORKER_DEFAULT_VALUE)


why we put default value as string and convert here? We can just define as int at the beginning.

@ChunxuTang previously put Alluxio related key=value string pair into the const file, we can discuss about what are the best way for dealing with property key and values

jja725 · 2024-02-20T21:52:30Z

alluxio/alluxio_file_system.py

        if options:
            if ALLUXIO_PAGE_SIZE_KEY in options:
                page_size = options[ALLUXIO_PAGE_SIZE_KEY]
                self.logger.debug(f"Page size is set to {page_size}")
+            if ALLUXIO_HASH_NODE_PER_WORKER_KEY in options:
+                hash_node_per_worker = int(


why not int?

The const is all string format, the const file is kind of Alluxio property key files now

jja725 · 2024-02-20T21:55:26Z

alluxio/worker_ring.py

+                worker_entity.worker_identity
+            ] = worker_entity.worker_net_address
+            if worker_entity.worker_identity not in self._worker_info_map:
+                diff_in_worker_info_detected = True


we can early break here if we detect diff.

worker_info_map[
worker_entity.worker_identity
] = worker_entity.worker_net_address

here we also need to update the local worker_info_map for update hash ring

LuQQiu · 2024-02-20T23:25:48Z

The docker related tests failed with cannot launch worker issues

ChunxuTang

Generally LGTM. Just left some minor comments.

ChunxuTang · 2024-02-20T22:07:05Z

alluxio/alluxio_file_system.py

@@ -141,20 +143,32 @@ def __init__(
                "Supply either 'etcd_hosts' or 'worker_hosts', not both"
            )
        self.logger = logger or logging.getLogger("AlluxioFileSystem")
+        if etcd_hosts and not worker_hosts:
+            self.logger.warning(
+                "Does not supply 'etcd_host'. Cannot tolerate Alluxio cluster changes."


etcd_host => etcd_hosts
Cannot tolerate Alluxio cluster changes. => A etcd cluster is required for dynamic cluster changes.

ChunxuTang · 2024-02-20T22:19:16Z

alluxio/worker_ring.py

-        self.host = host
-        self.port = port
+        self._host = host
+        self._port = port


Sure! Thanks.

LuQQiu added 3 commits February 6, 2024 14:28

Add initial hash ring update logics

e528626

Fix values

bfcd30e

Add worker hash ring test file and worker list source file

d23a344

LuQQiu and others added 12 commits February 7, 2024 13:35

Update

9cb8b31

update hasher and fix test

c926fab

ignore async test for now

8b7bb40

Test worked

1848063

support setting Alluxio cluster name

e264021

Merge

bfa42ab

Fix

274ff78

Merge branch 'addClusterName' into testWorkerHash

dfc787d

merge

a5fe24c

Refactor

9dd830c

Small fix

fadc23a

Modify worker hash

5b82edd

LuQQiu changed the title ~~[WIP] Test worker hash ring~~ Update worker hash ring Feb 15, 2024

LuQQiu requested review from jja725 and ChunxuTang February 15, 2024 23:08

LuQQiu added 2 commits February 15, 2024 15:25

Skip docker test for now

b1b8d8e

Skip docker test for now

119f4c2

ChunxuTang reviewed Feb 16, 2024

View reviewed changes

LuQQiu added 2 commits February 20, 2024 12:44

Address comments

b5bd9e4

Add initalize static membership service test

421461a

jja725 reviewed Feb 20, 2024

View reviewed changes

LuQQiu added 2 commits February 20, 2024 14:44

Support staticMembership

af1d8b4

Add test file

79bec76

ChunxuTang reviewed Feb 20, 2024

View reviewed changes

Address comments

60f7531

ChunxuTang approved these changes Feb 21, 2024

View reviewed changes

LuQQiu merged commit 6c76b86 into Alluxio:dev Feb 21, 2024
1 of 3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update worker hash ring #58

Update worker hash ring #58

LuQQiu commented Feb 7, 2024 •

edited

Loading

LuQQiu commented Feb 7, 2024

ChunxuTang left a comment

ChunxuTang Feb 16, 2024

LuQQiu Feb 20, 2024

ChunxuTang Feb 16, 2024

ChunxuTang Feb 16, 2024

LuQQiu Feb 16, 2024

LuQQiu Feb 20, 2024

ChunxuTang Feb 20, 2024

ChunxuTang Feb 16, 2024

ChunxuTang Feb 16, 2024

ChunxuTang Feb 16, 2024

jja725 left a comment

jja725 Feb 20, 2024

jja725 Feb 20, 2024

LuQQiu Feb 20, 2024

jja725 Feb 20, 2024

LuQQiu Feb 20, 2024

jja725 Feb 20, 2024

LuQQiu Feb 20, 2024

LuQQiu commented Feb 20, 2024

ChunxuTang left a comment

ChunxuTang Feb 20, 2024

ChunxuTang Feb 20, 2024

		worker_hosts (str, optional):
		The worker hostnames in host1,host2,host3 format. Either etcd_hosts or worker_hosts should be provided, not both.

		self.logger = logger or logging.getLogger("AlluxioFileSystem")
		if etcd_hosts and not worker_hosts:

Update worker hash ring #58

Update worker hash ring #58

Conversation

LuQQiu commented Feb 7, 2024 • edited Loading

LuQQiu commented Feb 7, 2024

ChunxuTang 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

jja725 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

LuQQiu commented Feb 20, 2024

ChunxuTang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LuQQiu commented Feb 7, 2024 •

edited

Loading