Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable static consistent hash ring #18183

Conversation

JiamingMai
Copy link
Contributor

@JiamingMai JiamingMai commented Sep 20, 2023

By default, we build a dynamic consistent hash with the live worker list that comes from master or ETCD. Sometimes we want to build a static consistent hash ring to make sure we won't write data to other worker node when a worker node is offline temporarily (especially when other worker nodes are running out of disk space).

This PR provides allows us to build a static consistent hash ring by setting alluxio.user.dynamic.consistent.hash.ring.enabled=false. In this case, client will read from UFS if the worker where the specified file locate is down.

@JiamingMai JiamingMai added the type-feature This issue is a feature request label Sep 20, 2023
@JiamingMai JiamingMai self-assigned this Sep 20, 2023
@JiamingMai JiamingMai force-pushed the return-full-worker-lists-with-active-label branch from c5d9377 to a9a80a8 Compare September 20, 2023 18:58
@lucyge2022
Copy link
Contributor

By default, we build a dynamic consistent hash with the live worker list that comes from master or ETCD. Sometimes we want to build a static consistent hash ring to make sure we won't write data to other worker node when a worker node is offline temporarily (especially when other worker nodes are running out of disk space).

This PR provides allows us to build a static consistent hash ring by setting alluxio.user.dynamic.consistent.hash.ring.enabled=false. In this case, client will read from UFS if the worker where the specified file locate is down.

actually if its etcd it has already been using a static consistent hash ring.

@JiamingMai
Copy link
Contributor Author

JiamingMai commented Sep 21, 2023

By default, we build a dynamic consistent hash with the live worker list that comes from master or ETCD. Sometimes we want to build a static consistent hash ring to make sure we won't write data to other worker node when a worker node is offline temporarily (especially when other worker nodes are running out of disk space).
This PR provides allows us to build a static consistent hash ring by setting alluxio.user.dynamic.consistent.hash.ring.enabled=false. In this case, client will read from UFS if the worker where the specified file locate is down.

actually if its etcd it has already been using a static consistent hash ring.

@lucyge2022 This PR makes us use dynamic consistent hash ring by default (no matter using ETCD or master). In this PR, I change the logic to get getLiveMembers instead of getAllMembers from ETCD by default. This allows users to choose the way they want.

@yyongycy
Copy link
Contributor

@jja725 wondering if the hash ring change impacts the logic of distributedMv related?

@jja725
Copy link
Contributor

jja725 commented Sep 21, 2023

@yyongycy this doesn't affect cp/mv, but probably affect distributed load, have to take a closer look.

@JiamingMai JiamingMai force-pushed the return-full-worker-lists-with-active-label branch from 27c41bb to 2e5c249 Compare September 21, 2023 02:17
@JiamingMai
Copy link
Contributor Author

@yyongycy this doesn't affect cp/mv, but probably affect distributed load, have to take a closer look.

Actually, we do want to affect distributed load. That is why we want to have this change. If there is a worker that is offline temporarily, static consistent hash ring allows user to avoid writing data to other worker nodes when executing load command and reading a file cached on the offline worker.

@jja725
Copy link
Contributor

jja725 commented Sep 21, 2023

@yyongycy this doesn't affect cp/mv, but probably affect distributed load, have to take a closer look.

Actually, we do want to affect distributed load. That is why we want to have this change. If there is a worker that is offline temporarily, static consistent hash ring allows user to avoid writing data to other worker nodes when executing load command and reading a file cached on the offline worker.

Then we can update DefaultWorkerProvider.getWorkerInfos

Copy link
Contributor

@yyongycy yyongycy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, looks like not a big changes. Master based worker registration would eventually be etcd based worker registration.

just need some data to see "time of the consistent hash ring building given it has 400K (100 physical nodes) vnodes"
Rest optimization can be done later.

@JiamingMai
Copy link
Contributor Author

@yyongycy this doesn't affect cp/mv, but probably affect distributed load, have to take a closer look.

Actually, we do want to affect distributed load. That is why we want to have this change. If there is a worker that is offline temporarily, static consistent hash ring allows user to avoid writing data to other worker nodes when executing load command and reading a file cached on the offline worker.

Then we can update DefaultWorkerProvider.getWorkerInfos

I updated DefaultWorkerProvider.getWorkerInfos. Please take a look when you have time. @jja725

@JiamingMai JiamingMai force-pushed the return-full-worker-lists-with-active-label branch 2 times, most recently from 27558f9 to f1c347c Compare September 21, 2023 08:13
@yyongycy
Copy link
Contributor

btw, please do test it.

Copy link
Contributor

@yyongycy yyongycy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm based on test done

@apc999
Copy link
Contributor

apc999 commented Sep 21, 2023

my understanding is that "master" mode is only used for backward purpose, will not be used for membership in the future.
correct?

@JiamingMai
Copy link
Contributor Author

my understanding is that "master" mode is only used for backward purpose, will not be used for membership in the future. correct?

Yes, it is only used for backward purpose. But some features still depends on master, for example load command requires master which has Scheduler for distributed load.

@jja725 jja725 self-requested a review September 22, 2023 20:49
Copy link
Contributor

@jja725 jja725 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, But I'm still a little fuzzy what's the exact behavior/exception when we lost a worker which is still running the task or send a request to lost worker. Do you think we can add some comment on those behaviors?

@JiamingMai
Copy link
Contributor Author

LGTM, But I'm still a little fuzzy what's the exact behavior/exception when we lost a worker which is still running the task or send a request to lost worker. Do you think we can add some comment on those behaviors?

The request will throw an exception after timeout. If we want to add a comment for this case, we need to identify the exact type of exception.

@JiamingMai JiamingMai force-pushed the return-full-worker-lists-with-active-label branch from 5f4b521 to 5fe21eb Compare September 24, 2023 15:49
@JiamingMai
Copy link
Contributor Author

alluxio-bot, merge this please

@alluxio-bot alluxio-bot merged commit 189ca24 into Alluxio:main Sep 24, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature This issue is a feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants