Use cluster view to replace list of workers #18451

dbw9580 · 2023-11-29T11:36:26Z

What changes are proposed in this pull request?

Replace List<BlockWorkerInfo> with WorkerClusterView in APIs.

Important APIs that are changed:

FileSystemContext.getCachedWorkers now returns WorkerClusterView
WorkerLocationPolicy.getPreferredWorkers (as well as all its implementations) now accepts a WorkerClusterView as the first argument (but still returns List<BlockWorkerInfo> as the returned list must be ordered)

APIs that are using List<BlockWorkerInfo> (or List<WorkerInfo>) but not migrated to WorkerClusterView:

alluxio.master.scheduler.WorkerProvider.getWorkerInfos returns List<WorkerInfo>.
Job service related APIs, e.g. alluxio.job.plan.PlanDefinition.selectExecutors

Notable behavior change:

Now EtcdMembershipManager assigns the correct state (LIVE or LOST) for all workers in its WorkerInfo struct. Before this change, this information is not available and the state defaults to UNRECOGNIZED.

Why are the changes needed?

Allow more efficient indexing and filtering workers by worker ID.

Does this PR introduce any user facing changes?

No.

dbw9580 · 2023-11-29T11:37:35Z

The branch depends on the other PR #18441 , will rebase once that one gets in.

# Conflicts: # dora/core/common/src/main/java/alluxio/membership/EtcdMembershipManager.java # dora/core/common/src/main/java/alluxio/membership/WorkerClusterView.java

dbw9580 · 2023-12-08T10:31:16Z

@lucyge2022 @JiamingMai @jiacheliu3 can you please take a look? thanks.

@jja725 I left the scheduler related APIs unchanged but I think it can also benefit from this refactor. Let me know what you think.

dbw9580 · 2023-12-08T10:33:52Z

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

+    Set<WorkerIdentity> liveWorkerIds = parseWorkersFromEtcdKvPairs(
+        mAlluxioEtcdClient.mServiceDiscovery.getAllLiveServices())
+        .map(WorkerServiceEntity::getIdentity)
+        .collect(Collectors.toSet());
+    Predicate<WorkerInfo> isLive = w -> liveWorkerIds.contains(w.getIdentity());


to figure out the liveness of a worker, I had to first get a set of all live workers, so that live and lost workers can be correctly differentiate. @lucyge2022 I'd appreciate your comment on this.

yeah u have to make separate calls to know the state of a worker

dbw9580 · 2023-12-08T10:43:05Z

dora/core/client/fs/src/main/java/alluxio/client/file/FileSystemContext.java

-        List<BlockWorkerInfo> liveWorkers = mMembershipManager.getLiveMembers().stream()
-            .map(w -> new BlockWorkerInfo(w.getIdentity(), w.getAddress(), w.getCapacityBytes(),
-                w.getUsedBytes(), true)).collect(toList());
-        List<BlockWorkerInfo> lostWorkers = mMembershipManager.getFailedMembers().stream()
-            .map(w -> new BlockWorkerInfo(
-                w.getIdentity(), w.getAddress(), w.getCapacityBytes(), w.getUsedBytes(),
-                false)).collect(toList());
-        // avoid duplicate elements in list
-        return combineAllWorkers(liveWorkers, lostWorkers);
+        return mMembershipManager.getAllMembers();


I was confused why we are taking trouble to combine a list of live workers with a list of lost workers to get the list of all workers, while all this time there is a getAllMembers method to do exactly this.

I think this change has a subtle difference, as the combineAllWorkers method used to de-duplicate workers by their net address. Now mMembershipManager.getAllMembers does so by checking the worker IDs. But workers shouldn't have conflicting net addresses either so it's no big problem IMO.

Agreed we can use mMembershipManager.getAllMembers given the subtle difference. But the property that workers shouldn't have conflicting net addresses is not guaranteed IMO? I suggest we double check that in the worker register code (in a separate PR if necessary). At least if some workers are using dup IDs or addresses we should check somewhere and log some warnings, so that people can realize at all.

workers shouldn't have conflicting net addresses is not guaranteed

Right. So there used to be some potential bugs due to conflicting worker addresses. But this is not relevant any more. Depending on net addresses to differentiate workers is not reliable, hence the introduction of worker IDs.

we double check that in the worker register code

I think Lucy did exactly that in #18454

@jiacheliu3 @dbw9580 actually if one worker register with workerid1 and then remove its workeridentity file and restart, it will register as workerid2, if no one removes workerid1 key from etcd, getAllMembers will have both worker entity bearing same addresses, but that's not sth code base should be guarding against, its a deployment issue. getAllMembers could be thought of as all the distinct members forming the ring.

dbw9580 · 2023-12-08T10:45:19Z

dora/core/client/fs/src/main/java/alluxio/client/file/dora/ConsistentHashPolicy.java

+    List<BlockWorkerInfo> blockWorkerInfoList = workerClusterView.stream()
+        .map(w -> new BlockWorkerInfo(w.getIdentity(), w.getAddress(), w.getCapacityBytes(),
+            w.getUsedBytes(), w.getState() == WorkerState.LIVE))
+        .collect(Collectors.toList());
+    HASH_PROVIDER.refresh(blockWorkerInfoList, mNumVirtualNodes);


refactoring ConsistentHashProvider to accept WorkerClusterView instead of List<BlockWorkerInfo> will be done in a separate PR: #18434

dbw9580 · 2023-12-08T10:46:30Z

dora/core/client/fs/src/main/java/alluxio/client/file/dora/WorkerLocationPolicy.java

   * @param fileId
   * @param count
   * @return a list of preferred workers
   * @throws ResourceExhaustedException if unable to return exactly #{count} workers
   */
-  List<BlockWorkerInfo> getPreferredWorkers(List<BlockWorkerInfo> blockWorkerInfos,
+  List<BlockWorkerInfo> getPreferredWorkers(WorkerClusterView workers,


the return type is unchanged because the order of the workers returned is important.

jiacheliu3

Mostly LGTM. Good to merge once the comments are looked at. Thanks for the work!

dora/core/client/fs/src/main/java/alluxio/client/file/FileSystemContext.java

jiacheliu3 · 2023-12-10T12:09:52Z

dora/core/client/fs/src/main/java/alluxio/client/file/FileSystemContext.java

          || mWorkerRefreshPolicy.attempt()) {
        switch (type) {
          case ALL:
-            mWorkerInfoList.set(getAllWorkers());
+            mCachedWorkerClusterView.set(getAllWorkers());


oh interestingly, this same cache object is caching results for 3 types? how do we know if a get wants the same type as is cached?

good catch! I think this is a potential bug. Since the only caller to this overloaded getCachedWorkers(GetWorkerListType) is DoraCacheClient.getWorkerNetAddress and it's argument GetWorkerListType is tied to mEnableDynamicHashRing which is currently a runtime constant, this does not cause any visible bug.

IMO since there are already getLiveWorkers getAllWorkers and getLostWorkers methods on FileSystemContext, I'd propose to simply deprecate/remove this method.

to fix this in a separate PR.

jiacheliu3 · 2023-12-10T12:17:11Z

dora/core/client/fs/src/main/java/alluxio/client/file/FileSystemContext.java

-        List<BlockWorkerInfo> liveWorkers = mMembershipManager.getLiveMembers().stream()
-            .map(w -> new BlockWorkerInfo(w.getIdentity(), w.getAddress(), w.getCapacityBytes(),
-                w.getUsedBytes(), true)).collect(toList());
-        List<BlockWorkerInfo> lostWorkers = mMembershipManager.getFailedMembers().stream()
-            .map(w -> new BlockWorkerInfo(
-                w.getIdentity(), w.getAddress(), w.getCapacityBytes(), w.getUsedBytes(),
-                false)).collect(toList());
-        // avoid duplicate elements in list
-        return combineAllWorkers(liveWorkers, lostWorkers);
+        return mMembershipManager.getAllMembers();


Agreed we can use mMembershipManager.getAllMembers given the subtle difference. But the property that workers shouldn't have conflicting net addresses is not guaranteed IMO? I suggest we double check that in the worker register code (in a separate PR if necessary). At least if some workers are using dup IDs or addresses we should check somewhere and log some warnings, so that people can realize at all.

# Conflicts: # dora/core/common/src/main/java/alluxio/membership/EtcdMembershipManager.java

lucyge2022 · 2023-12-11T19:30:57Z

dora/core/client/fs/src/main/java/alluxio/client/file/FileSystemContext.java

+  /**
+   * The policy to refresh workers list.
+   */
+  @GuardedBy("mCachedWorkerClusterView")


why is this policy guarded by the workerlist? there's no update to this policy? can u remove this if its a legacy thing

RefreshPolicy objects are not thread safe, and TimeoutRefresh.attempt actually mutates its internal state. Therefore access to it must be serialized across different threads.

This policy object is only used in getCachedWorkers so it's fine to guard it with mCachedWorkerClusterView's lock.

dora/core/client/fs/src/main/java/alluxio/client/file/dora/ConsistentHashPolicy.java

lucyge2022

LGTM other than some minor comments

dbw9580 · 2023-12-12T07:51:48Z

alluxio-bot, merge this please

alluxio-bot · 2023-12-12T07:51:50Z

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

dbw9580 · 2023-12-12T07:52:57Z

alluxio-bot, merge this please

### What changes are proposed in this pull request? Replace `List<BlockWorkerInfo>` with `WorkerClusterView` in APIs. Important APIs that are changed: 1. `FileSystemContext.getCachedWorkers` now returns `WorkerClusterView` 2. `WorkerLocationPolicy.getPreferredWorkers` (as well as all its implementations) now accepts a `WorkerClusterView` as the first argument (but still returns `List<BlockWorkerInfo>` as the returned list must be ordered) APIs that are using `List<BlockWorkerInfo>` (or `List<WorkerInfo>`) but *not* migrated to `WorkerClusterView`: 1. `alluxio.master.scheduler.WorkerProvider.getWorkerInfos` returns `List<WorkerInfo>`. 2. Job service related APIs, e.g. `alluxio.job.plan.PlanDefinition.selectExecutors` Notable behavior change: Now `EtcdMembershipManager` assigns the correct state (`LIVE` or `LOST`) for all workers in its `WorkerInfo` struct. Before this change, this information is not available and the state defaults to `UNRECOGNIZED`. ### Why are the changes needed? Allow more efficient indexing and filtering workers by worker ID. ### Does this PR introduce any user facing changes? No. pr-link: Alluxio#18451 change-id: cid-5052d2faa506f4de6e4b0df7062c5def3e09df1c

dbw9580 added 13 commits November 24, 2023 19:00

add WorkerClusterView

52a6809

simplify WorkerClusterView

7b87e57

fix compilation

3753cfa

rename

e726b5f

add creation timestamp to snapshot

5900fc3

add size and isEmpty methods

b5bd290

address comments

8b22649

revert changes

b7bec81

checkstyle

fec7441

move WorkerState to wire package

dfb0259

rename ACTIVE to LIVE

e025221

turn worker state from string to enum in WorkerInfo

82e58b4

replace list of worker info with WorkerClusterView

dfe5881

dbw9580 requested review from jiacheliu3 and lucyge2022 November 29, 2023 11:37

dbw9580 added 8 commits December 5, 2023 14:08

Merge branch 'dora' into feat/use-cluster-view

539325b

# Conflicts: # dora/core/common/src/main/java/alluxio/membership/EtcdMembershipManager.java # dora/core/common/src/main/java/alluxio/membership/WorkerClusterView.java

add unrecognized worker state and set as default in worker indo

74fa9fd

add copy constructors to WorkerInfo and WorkerNetAddress

6bbe3e8

fix test

c619492

fix test

de28c4b

checkstyle

2284d53

fix NPE

15a4092

Merge branch 'dora' into feat/use-cluster-view

4e9352b

dbw9580 commented Dec 8, 2023

View reviewed changes

jiacheliu3 approved these changes Dec 10, 2023

View reviewed changes

dbw9580 added 2 commits December 11, 2023 15:40

address comments

5887430

Merge branch 'dora' into feat/use-cluster-view

a8617af

# Conflicts: # dora/core/common/src/main/java/alluxio/membership/EtcdMembershipManager.java

lucyge2022 reviewed Dec 11, 2023

View reviewed changes

dora/core/client/fs/src/main/java/alluxio/client/file/dora/ConsistentHashPolicy.java Show resolved Hide resolved

lucyge2022 approved these changes Dec 11, 2023

View reviewed changes

dbw9580 added the type-code-quality code quality improvement label Dec 12, 2023

alluxio-bot merged commit 9eae1e9 into Alluxio:main Dec 12, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use cluster view to replace list of workers #18451

Use cluster view to replace list of workers #18451

dbw9580 commented Nov 29, 2023 •

edited

Loading

dbw9580 commented Nov 29, 2023

dbw9580 commented Dec 8, 2023

dbw9580 Dec 8, 2023

lucyge2022 Dec 11, 2023

dbw9580 Dec 8, 2023

jiacheliu3 Dec 10, 2023

dbw9580 Dec 11, 2023

lucyge2022 Dec 11, 2023

dbw9580 Dec 8, 2023

dbw9580 Dec 8, 2023

jiacheliu3 left a comment

jiacheliu3 Dec 10, 2023

dbw9580 Dec 11, 2023

dbw9580 Dec 11, 2023

lucyge2022 Dec 11, 2023

jiacheliu3 Dec 10, 2023

lucyge2022 Dec 11, 2023

dbw9580 Dec 12, 2023

lucyge2022 left a comment

dbw9580 commented Dec 12, 2023

alluxio-bot commented Dec 12, 2023

dbw9580 commented Dec 12, 2023

Use cluster view to replace list of workers #18451

Use cluster view to replace list of workers #18451

Conversation

dbw9580 commented Nov 29, 2023 • edited Loading

What changes are proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user facing changes?

dbw9580 commented Nov 29, 2023

dbw9580 commented Dec 8, 2023

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

jiacheliu3 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

lucyge2022 left a comment

Choose a reason for hiding this comment

dbw9580 commented Dec 12, 2023

alluxio-bot commented Dec 12, 2023

dbw9580 commented Dec 12, 2023

dbw9580 commented Nov 29, 2023 •

edited

Loading