-
Notifications
You must be signed in to change notification settings - Fork 361
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
[CELEBORN-1675] User with zero quota should not be allowed to submit any shuffle to cluster. #2857
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Please update the config docs to reflect the same.
updated |
@Z1Wu You need to update the documentation as well using
Also, maintainers need to enable the PR workflows for this PR. ping @SteNicholas @RexXiong @FMX PTAL |
updated, thanks for your review. |
@@ -57,7 +57,7 @@ class QuotaManager(celebornConf: CelebornConf, configService: ConfigService) ext | |||
userIdentifier: UserIdentifier, | |||
value: Long, | |||
quota: Quota): (Boolean, String) = { | |||
val exceed = (quota.diskBytesWritten > 0 && value >= quota.diskBytesWritten) | |||
val exceed = (quota.diskBytesWritten >= 0 && value >= quota.diskBytesWritten) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that if the value is negative, it indicates that the quota is unlimited, which feels a bit counterintuitive. For numbers less than or equal to 0, it seems better to directly determine them as having no quota.
cc @leixm @AngersZhuuuu
This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
What changes were proposed in this pull request?
In current implementation, user with zero quota still can submit one shuffle to rss cluster.
It might be more reasonable to disallow users with a quota of 0 from submitting a shuffle to the RSS cluster.
Why are the changes needed?
Does this PR introduce any user-facing change?
No
How was this patch tested?
unit-test