You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "AngersZhuuuu (via GitHub)" <gi...@apache.org> on 2024/02/22 10:25:21 UTC
[PR] [CELEBORN-1276] Move checkQuotaSpaceAvailable from Quota to QuotaManager [incubator-celeborn]
AngersZhuuuu opened a new pull request, #2317:
URL: https://github.com/apache/incubator-celeborn/pull/2317
<!--
Thanks for sending a pull request! Here are some tips for you:
- Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
- Be sure to keep the PR description updated to reflect all changes.
- Please write your PR title to summarize what this PR proposes.
- If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
Move checkQuotaSpaceAvailable from Quota to QuotaManager
### Why are the changes needed?
Put method in correct place
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Added UT
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1276] Move checkQuotaSpaceAvailable from Quota to QuotaManager [incubator-celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2317:
URL: https://github.com/apache/incubator-celeborn/pull/2317#issuecomment-1959168467
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2317?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`0c952ca`)](https://app.codecov.io/gh/apache/incubator-celeborn/commit/0c952ca915a592176b85abd273030567653a6f18?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 48.87% compared to head [(`4d3473f`)](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2317?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 49.00%.
> :exclamation: Current head 4d3473f differs from pull request most recent head 015b46d. Consider uploading reports for the commit 015b46d to get more accurate results
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #2317 +/- ##
==========================================
+ Coverage 48.87% 49.00% +0.13%
==========================================
Files 207 207
Lines 12936 12902 -34
Branches 1114 1109 -5
==========================================
Hits 6321 6321
+ Misses 6208 6174 -34
Partials 407 407
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2317?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1276] Move checkQuotaSpaceAvailable from Quota to QuotaManager [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2317:
URL: https://github.com/apache/incubator-celeborn/pull/2317#discussion_r1500157307
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/quota/QuotaManager.scala:
##########
@@ -37,4 +38,74 @@ class QuotaManager(celebornConf: CelebornConf, configService: ConfigService) ext
DEFAULT_QUOTA
}
}
+
+ def checkQuotaSpaceAvailable(
+ userIdentifier: UserIdentifier,
+ resourceResumption: ResourceConsumption): (Boolean, String) = {
+ val quota = getQuota(userIdentifier)
+ val checkResults = Seq(
+ checkDiskBytesWritten(userIdentifier, resourceResumption.diskBytesWritten, quota),
+ checkDiskFileCount(userIdentifier, resourceResumption.diskFileCount, quota),
+ checkHdfsBytesWritten(userIdentifier, resourceResumption.hdfsBytesWritten, quota),
+ checkHdfsFileCount(userIdentifier, resourceResumption.hdfsFileCount, quota))
+ val exceed = checkResults.foldLeft(false)(_ || _._1)
+ val reason = checkResults.foldLeft("")(_ + _._2)
+ (!exceed, reason)
+ }
+
+ private def checkDiskBytesWritten(
Review Comment:
Could the check methods merge into one method? Most implementation of check methods are similar.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1276] Move checkQuotaSpaceAvailable from Quota to QuotaManager [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on PR #2317:
URL: https://github.com/apache/incubator-celeborn/pull/2317#issuecomment-1960649907
Thanks, merge to main(v0.5.0)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1276] Move checkQuotaSpaceAvailable from Quota to QuotaManager [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on PR #2317:
URL: https://github.com/apache/incubator-celeborn/pull/2317#issuecomment-1959141011
ping @RexXiong
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1276] Move checkQuotaSpaceAvailable from Quota to QuotaManager [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu closed pull request #2317: [CELEBORN-1276] Move checkQuotaSpaceAvailable from Quota to QuotaManager
URL: https://github.com/apache/incubator-celeborn/pull/2317
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1276] Move checkQuotaSpaceAvailable from Quota to QuotaManager [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #2317:
URL: https://github.com/apache/incubator-celeborn/pull/2317#discussion_r1500163512
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/quota/QuotaManager.scala:
##########
@@ -37,4 +38,74 @@ class QuotaManager(celebornConf: CelebornConf, configService: ConfigService) ext
DEFAULT_QUOTA
}
}
+
+ def checkQuotaSpaceAvailable(
+ userIdentifier: UserIdentifier,
+ resourceResumption: ResourceConsumption): (Boolean, String) = {
+ val quota = getQuota(userIdentifier)
+ val checkResults = Seq(
+ checkDiskBytesWritten(userIdentifier, resourceResumption.diskBytesWritten, quota),
+ checkDiskFileCount(userIdentifier, resourceResumption.diskFileCount, quota),
+ checkHdfsBytesWritten(userIdentifier, resourceResumption.hdfsBytesWritten, quota),
+ checkHdfsFileCount(userIdentifier, resourceResumption.hdfsFileCount, quota))
+ val exceed = checkResults.foldLeft(false)(_ || _._1)
+ val reason = checkResults.foldLeft("")(_ + _._2)
+ (!exceed, reason)
+ }
+
+ private def checkDiskBytesWritten(
Review Comment:
> Could the check methods merge into one method? Most implementation of check methods are similar.
Let me do a final refactor pr for all code related to quota management.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org