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/18 09:50:39 UTC
[PR] [CELEBORN-1239][FEATURE] Celeborn QuotaManager support use FsConfigService and support default quota setting [incubator-celeborn]
AngersZhuuuu opened a new pull request, #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298
### What changes were proposed in this pull request?
This pr does 2 things:
1. Move the quota manager to Master side since only master use this
2. Support quota manager use FsConfigService and support default system level
### Why are the changes needed?
1. Many times, for users who do not have a quota configured, we hope to have a default quota that applies to them.
2. Quota manager should support refresh
3. QuotaManager should support integrate with ConfigService
### 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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#issuecomment-1951173076
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2298?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 [(`e4f7ea8`)](https://app.codecov.io/gh/apache/incubator-celeborn/commit/e4f7ea8e01c9f8460254d3d13aee5a920a3822ab?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 48.92% compared to head [(`a6b1270`)](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2298?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 49.08%.
> Report is 2 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #2298 +/- ##
==========================================
+ Coverage 48.92% 49.08% +0.17%
==========================================
Files 208 206 -2
Lines 12893 12857 -36
Branches 1113 1111 -2
==========================================
+ Hits 6306 6309 +3
+ Misses 6178 6140 -38
+ Partials 409 408 -1
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2298?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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#discussion_r1497276318
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/quota/QuotaManager.scala:
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.celeborn.service.deploy.master.quota
+
+import org.apache.celeborn.common.CelebornConf
+import org.apache.celeborn.common.CelebornConf.{QUOTA_DISK_BYTES_WRITTEN, QUOTA_DISK_FILE_COUNT, QUOTA_HDFS_BYTES_WRITTEN, QUOTA_HDFS_FILE_COUNT}
+import org.apache.celeborn.common.identity.UserIdentifier
+import org.apache.celeborn.common.internal.Logging
+import org.apache.celeborn.common.internal.config.ConfigEntry
+import org.apache.celeborn.common.quota.Quota
+import org.apache.celeborn.server.common.service.config.{DynamicConfig, DynamicConfigServiceFactory}
+import org.apache.celeborn.server.common.service.config.DynamicConfig.ConfigType
+
+class QuotaManager(celebornConf: CelebornConf) extends Logging {
+ val configService = DynamicConfigServiceFactory.getConfigService(celebornConf)
+ def getQuota(userIdentifier: UserIdentifier): Quota = {
+ if (configService != null) {
+ val config =
+ configService.getTenantUserConfigFromCache(userIdentifier.tenantId, userIdentifier.name)
+ getQuota(config)
+ } else {
+ Quota.DEFAULT_QUOTA
Review Comment:
Done
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#issuecomment-1953567569
ping @RexXiong @SteNicholas Pls take a look again
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#discussion_r1498578951
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4161,6 +4160,8 @@ object CelebornConf extends Logging {
.booleanConf
.createWithDefault(true)
+ val DEFAULT_QUOTA = new Quota()
Review Comment:
I think default should be no limit.
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#discussion_r1494493596
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4203,10 +4202,11 @@ object CelebornConf extends Logging {
val QUOTA_MANAGER: ConfigEntry[String] =
Review Comment:
We now already have backend option(FS, DB), so IMO We only need a QuotaManager(ConfigService) for different backend.
##########
common/src/test/scala/org/apache/celeborn/ConfigurationSuite.scala:
##########
@@ -86,6 +86,10 @@ class ConfigurationSuite extends AnyFunSuite {
generateConfigurationMarkdown("ha")
}
+ test("docs - configuration/dynamic-quota.md") {
Review Comment:
may be quota.md
##########
master/src/test/resources/celeborn-0.5.0-h2-quota-ut-data.sql:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+INSERT INTO celeborn_cluster_info ( `id`, `name`, `namespace`, `endpoint`, `gmt_create`, `gmt_modify` )
Review Comment:
I suggest we just mock config service for quota manager instead to use duplicate sql files in ut.
##########
master/pom.xml:
##########
@@ -88,6 +88,21 @@
<artifactId>mockito-core</artifactId>
<scope>test</scope>
</dependency>
+
+ <!-- Db dependencies -->
+ <dependency>
+ <groupId>org.mybatis</groupId>
Review Comment:
IMO we don't need mybatis/HikariCp dependency
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4217,6 +4217,38 @@ object CelebornConf extends Logging {
.stringConf
.createOptional
+ val QUOTA_DISK_BYTES_WRITTEN: ConfigEntry[Long] =
+ buildConf("celeborn.quota.diskBytesWritten")
+ .categories("dynamic-quota")
Review Comment:
IMO categories may be quota/master, we may add a dynamic property for ConfigEntry
`| Key | Default | isDynamic| Description | Since | Deprecated `
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#discussion_r1498861464
##########
service/src/main/java/org/apache/celeborn/server/common/service/config/SystemConfig.java:
##########
@@ -31,16 +31,19 @@ public class SystemConfig extends DynamicConfig {
public SystemConfig(CelebornConf celebornConf, Map<String, String> configs) {
this.celebornConf = celebornConf;
this.configs.putAll(configs);
+ this.quota = currentQuota();
Review Comment:
> Better use lazy initialization for quota
Done
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#issuecomment-1951626791
Ping @RexXiong Pls take a look
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#discussion_r1498544822
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4161,6 +4160,8 @@ object CelebornConf extends Logging {
.booleanConf
.createWithDefault(true)
+ val DEFAULT_QUOTA = new Quota()
Review Comment:
You may have misunderstood my meaning, I think the DEFAULT_QUOTA should be `new Quota(QUOTA_DISK_BYTES_WRITTEN.value, QUOTA_DISK_FILE_COUNT.value ....)`, and the default value may be
a large value or negative values means no limit on the quota , WDYT @AngersZhuuuu?
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#discussion_r1495208703
##########
common/src/test/scala/org/apache/celeborn/ConfigurationSuite.scala:
##########
@@ -86,6 +86,10 @@ class ConfigurationSuite extends AnyFunSuite {
generateConfigurationMarkdown("ha")
}
+ test("docs - configuration/dynamic-quota.md") {
Review Comment:
Rmeoved
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#discussion_r1498818785
##########
service/src/main/java/org/apache/celeborn/server/common/service/config/SystemConfig.java:
##########
@@ -31,16 +31,19 @@ public class SystemConfig extends DynamicConfig {
public SystemConfig(CelebornConf celebornConf, Map<String, String> configs) {
this.celebornConf = celebornConf;
this.configs.putAll(configs);
+ this.quota = currentQuota();
Review Comment:
Better use lazy initialization for quota
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4216,6 +4206,42 @@ object CelebornConf extends Logging {
.stringConf
.createOptional
+ val QUOTA_DISK_BYTES_WRITTEN: ConfigEntry[Long] =
+ buildConf("celeborn.quota.diskBytesWritten")
Review Comment:
Shall we add a tenant field for these configurations? such as celeborn.quota.tenant.diskBytesWritten, to be clear these configurations are for multi-tenancy
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/quota/QuotaManager.scala:
##########
@@ -14,35 +14,27 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-
-package org.apache.celeborn.common.quota
-
-import org.junit.Assert.assertEquals
+package org.apache.celeborn.service.deploy.master.quota
import org.apache.celeborn.common.CelebornConf
import org.apache.celeborn.common.identity.UserIdentifier
-import org.apache.celeborn.common.util.Utils
-
-class DefaultQuotaManagerSuite extends BaseQuotaManagerSuite {
-
- override def beforeAll(): Unit = {
- val conf = new CelebornConf()
- conf.set(
- CelebornConf.QUOTA_CONFIGURATION_PATH.key,
- getTestResourceFile("test-quota.yaml").getPath)
- quotaManager = QuotaManager.instantiate(conf)
- }
-
- test("initialize QuotaManager") {
- assert(quotaManager.isInstanceOf[DefaultQuotaManager])
- }
+import org.apache.celeborn.common.internal.Logging
+import org.apache.celeborn.common.quota.Quota
+import org.apache.celeborn.server.common.service.config.ConfigService
- test("test celeborn quota conf") {
- assertEquals(
- quotaManager.getQuota(UserIdentifier("AAA", "Tom")),
- Quota(Utils.byteStringAsBytes("100m"), 200, -1, -1))
- assertEquals(
- quotaManager.getQuota(UserIdentifier("BBB", "Jerry")),
- Quota(-1, -1, Utils.byteStringAsBytes("200m"), 200))
+class QuotaManager(celebornConf: CelebornConf, configService: ConfigService) extends Logging {
+ val DEFAULT_QUOTA = Quota(
Review Comment:
IMO move checkQuotaSpaceAvailable from Quota to QuotaManager is more better for quota manage and check, we also can add a configuration at service side to switch quota check enable/disable. It can refactor in next prs.
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#discussion_r1498856715
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4216,6 +4206,42 @@ object CelebornConf extends Logging {
.stringConf
.createOptional
+ val QUOTA_DISK_BYTES_WRITTEN: ConfigEntry[Long] =
+ buildConf("celeborn.quota.diskBytesWritten")
Review Comment:
> Shall we add a tenant field for these configurations? such as celeborn.quota.tenant.diskBytesWritten, to be clear these configurations are for multi-tenancy
But system also use this....
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#discussion_r1498568093
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4161,6 +4160,8 @@ object CelebornConf extends Logging {
.booleanConf
.createWithDefault(true)
+ val DEFAULT_QUOTA = new Quota()
Review Comment:
How about current?
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#discussion_r1498869284
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4216,6 +4206,42 @@ object CelebornConf extends Logging {
.stringConf
.createOptional
+ val QUOTA_DISK_BYTES_WRITTEN: ConfigEntry[Long] =
+ buildConf("celeborn.quota.diskBytesWritten")
Review Comment:
> > > Shall we add a tenant field for these configurations? such as celeborn.quota.tenant.diskBytesWritten, to be clear these configurations are for multi-tenancy
> >
> >
> > But system also use this....
>
> These system level configuration is also for tenants(not for an unique tenant)
Ok， done
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#discussion_r1497024166
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/quota/QuotaManager.scala:
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.celeborn.service.deploy.master.quota
+
+import org.apache.celeborn.common.CelebornConf
+import org.apache.celeborn.common.CelebornConf.{QUOTA_DISK_BYTES_WRITTEN, QUOTA_DISK_FILE_COUNT, QUOTA_HDFS_BYTES_WRITTEN, QUOTA_HDFS_FILE_COUNT}
+import org.apache.celeborn.common.identity.UserIdentifier
+import org.apache.celeborn.common.internal.Logging
+import org.apache.celeborn.common.internal.config.ConfigEntry
+import org.apache.celeborn.common.quota.Quota
+import org.apache.celeborn.server.common.service.config.{DynamicConfig, DynamicConfigServiceFactory}
+import org.apache.celeborn.server.common.service.config.DynamicConfig.ConfigType
+
+class QuotaManager(celebornConf: CelebornConf) extends Logging {
+ val configService = DynamicConfigServiceFactory.getConfigService(celebornConf)
Review Comment:
> Should we setting DYNAMIC_CONFIG_STORE_BACKEND to `FS` as default? If so we also need to have file not exist issue. cc @RexXiong
I've thought about this before, set `FS` as default may cause problems when users upgrade, otherwise IMO setting `FS` as default is more better.
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/quota/QuotaManager.scala:
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.celeborn.service.deploy.master.quota
+
+import org.apache.celeborn.common.CelebornConf
+import org.apache.celeborn.common.CelebornConf.{QUOTA_DISK_BYTES_WRITTEN, QUOTA_DISK_FILE_COUNT, QUOTA_HDFS_BYTES_WRITTEN, QUOTA_HDFS_FILE_COUNT}
+import org.apache.celeborn.common.identity.UserIdentifier
+import org.apache.celeborn.common.internal.Logging
+import org.apache.celeborn.common.internal.config.ConfigEntry
+import org.apache.celeborn.common.quota.Quota
+import org.apache.celeborn.server.common.service.config.{DynamicConfig, DynamicConfigServiceFactory}
+import org.apache.celeborn.server.common.service.config.DynamicConfig.ConfigType
+
+class QuotaManager(celebornConf: CelebornConf) extends Logging {
+ val configService = DynamicConfigServiceFactory.getConfigService(celebornConf)
+ def getQuota(userIdentifier: UserIdentifier): Quota = {
+ if (configService != null) {
+ val config =
+ configService.getTenantUserConfigFromCache(userIdentifier.tenantId, userIdentifier.name)
+ getQuota(config)
+ } else {
+ Quota.DEFAULT_QUOTA
Review Comment:
Default Quota should be from CelebornConf?
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/quota/QuotaManager.scala:
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.celeborn.service.deploy.master.quota
+
+import org.apache.celeborn.common.CelebornConf
+import org.apache.celeborn.common.CelebornConf.{QUOTA_DISK_BYTES_WRITTEN, QUOTA_DISK_FILE_COUNT, QUOTA_HDFS_BYTES_WRITTEN, QUOTA_HDFS_FILE_COUNT}
+import org.apache.celeborn.common.identity.UserIdentifier
+import org.apache.celeborn.common.internal.Logging
+import org.apache.celeborn.common.internal.config.ConfigEntry
+import org.apache.celeborn.common.quota.Quota
+import org.apache.celeborn.server.common.service.config.{DynamicConfig, DynamicConfigServiceFactory}
+import org.apache.celeborn.server.common.service.config.DynamicConfig.ConfigType
+
+class QuotaManager(celebornConf: CelebornConf) extends Logging {
+ val configService = DynamicConfigServiceFactory.getConfigService(celebornConf)
Review Comment:
ConfigService may initialize in Master, as it would be used somewhere els.
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#discussion_r1495174433
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4217,6 +4217,38 @@ object CelebornConf extends Logging {
.stringConf
.createOptional
+ val QUOTA_DISK_BYTES_WRITTEN: ConfigEntry[Long] =
+ buildConf("celeborn.quota.diskBytesWritten")
+ .categories("dynamic-quota")
Review Comment:
Done
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#discussion_r1495208703
##########
common/src/test/scala/org/apache/celeborn/ConfigurationSuite.scala:
##########
@@ -86,6 +86,10 @@ class ConfigurationSuite extends AnyFunSuite {
generateConfigurationMarkdown("ha")
}
+ test("docs - configuration/dynamic-quota.md") {
Review Comment:
Already have quota.md, removed
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#discussion_r1497266354
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/quota/QuotaManager.scala:
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.celeborn.service.deploy.master.quota
+
+import org.apache.celeborn.common.CelebornConf
+import org.apache.celeborn.common.CelebornConf.{QUOTA_DISK_BYTES_WRITTEN, QUOTA_DISK_FILE_COUNT, QUOTA_HDFS_BYTES_WRITTEN, QUOTA_HDFS_FILE_COUNT}
+import org.apache.celeborn.common.identity.UserIdentifier
+import org.apache.celeborn.common.internal.Logging
+import org.apache.celeborn.common.internal.config.ConfigEntry
+import org.apache.celeborn.common.quota.Quota
+import org.apache.celeborn.server.common.service.config.{DynamicConfig, DynamicConfigServiceFactory}
+import org.apache.celeborn.server.common.service.config.DynamicConfig.ConfigType
+
+class QuotaManager(celebornConf: CelebornConf) extends Logging {
+ val configService = DynamicConfigServiceFactory.getConfigService(celebornConf)
Review Comment:
> ConfigService may initialize in Master, as it would be used somewhere els.
Done, keep same with https://github.com/apache/incubator-celeborn/pull/2311
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#discussion_r1498860066
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4216,6 +4206,42 @@ object CelebornConf extends Logging {
.stringConf
.createOptional
+ val QUOTA_DISK_BYTES_WRITTEN: ConfigEntry[Long] =
+ buildConf("celeborn.quota.diskBytesWritten")
Review Comment:
> > Shall we add a tenant field for these configurations? such as celeborn.quota.tenant.diskBytesWritten, to be clear these configurations are for multi-tenancy
>
> But system also use this....
These system level configuration is also for tenants(not for an unique tenant)
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu closed pull request #2298: [CELEBORN-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting
URL: https://github.com/apache/incubator-celeborn/pull/2298
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#discussion_r1498910763
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/quota/QuotaManager.scala:
##########
@@ -14,35 +14,27 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-
-package org.apache.celeborn.common.quota
-
-import org.junit.Assert.assertEquals
+package org.apache.celeborn.service.deploy.master.quota
import org.apache.celeborn.common.CelebornConf
import org.apache.celeborn.common.identity.UserIdentifier
-import org.apache.celeborn.common.util.Utils
-
-class DefaultQuotaManagerSuite extends BaseQuotaManagerSuite {
-
- override def beforeAll(): Unit = {
- val conf = new CelebornConf()
- conf.set(
- CelebornConf.QUOTA_CONFIGURATION_PATH.key,
- getTestResourceFile("test-quota.yaml").getPath)
- quotaManager = QuotaManager.instantiate(conf)
- }
-
- test("initialize QuotaManager") {
- assert(quotaManager.isInstanceOf[DefaultQuotaManager])
- }
+import org.apache.celeborn.common.internal.Logging
+import org.apache.celeborn.common.quota.Quota
+import org.apache.celeborn.server.common.service.config.ConfigService
- test("test celeborn quota conf") {
- assertEquals(
- quotaManager.getQuota(UserIdentifier("AAA", "Tom")),
- Quota(Utils.byteStringAsBytes("100m"), 200, -1, -1))
- assertEquals(
- quotaManager.getQuota(UserIdentifier("BBB", "Jerry")),
- Quota(-1, -1, Utils.byteStringAsBytes("200m"), 200))
+class QuotaManager(celebornConf: CelebornConf, configService: ConfigService) extends Logging {
+ val DEFAULT_QUOTA = Quota(
Review Comment:
Yea, will done in next pr
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#discussion_r1495320198
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/quota/QuotaManager.scala:
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.celeborn.service.deploy.master.quota
+
+import org.apache.celeborn.common.CelebornConf
+import org.apache.celeborn.common.CelebornConf.{QUOTA_DISK_BYTES_WRITTEN, QUOTA_DISK_FILE_COUNT, QUOTA_HDFS_BYTES_WRITTEN, QUOTA_HDFS_FILE_COUNT}
+import org.apache.celeborn.common.identity.UserIdentifier
+import org.apache.celeborn.common.internal.Logging
+import org.apache.celeborn.common.internal.config.ConfigEntry
+import org.apache.celeborn.common.quota.Quota
+import org.apache.celeborn.server.common.service.config.{DynamicConfig, DynamicConfigServiceFactory}
+import org.apache.celeborn.server.common.service.config.DynamicConfig.ConfigType
+
+class QuotaManager(celebornConf: CelebornConf) extends Logging {
+ val configService = DynamicConfigServiceFactory.getConfigService(celebornConf)
Review Comment:
Should we setting DYNAMIC_CONFIG_STORE_BACKEND to `FS` as default?
If so we also need to have file not exist issue. cc @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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#issuecomment-1959094741
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#discussion_r1495168869
##########
master/src/test/resources/celeborn-0.5.0-h2-quota-ut-data.sql:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+INSERT INTO celeborn_cluster_info ( `id`, `name`, `namespace`, `endpoint`, `gmt_create`, `gmt_modify` )
Review Comment:
Not need yet
##########
master/pom.xml:
##########
@@ -88,6 +88,21 @@
<artifactId>mockito-core</artifactId>
<scope>test</scope>
</dependency>
+
+ <!-- Db dependencies -->
+ <dependency>
+ <groupId>org.mybatis</groupId>
Review Comment:
Removed
--
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-1239][FEATURE] Celeborn QuotaManager support use ConfigService and support default quota setting [incubator-celeborn]
Posted by "AngersZhuuuu (via GitHub)" <gi...@apache.org>.
AngersZhuuuu commented on code in PR #2298:
URL: https://github.com/apache/incubator-celeborn/pull/2298#discussion_r1495169057
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4203,10 +4202,11 @@ object CelebornConf extends Logging {
val QUOTA_MANAGER: ConfigEntry[String] =
Review Comment:
Done
--
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