You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "RexXiong (via GitHub)" <gi...@apache.org> on 2024/01/30 10:01:42 UTC
[PR] [CELEBORN-1045] Support db based dynamic config server [incubator-celeborn]
RexXiong opened a new pull request, #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273
<!--
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?
### Why are the changes needed?
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475867307
##########
service/src/main/java/org/apache/celeborn/server/common/service/store/db/mapper/ClusterSystemConfigMapper.java:
##########
@@ -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.
+ */
+
+package org.apache.celeborn.server.common.service.store.db.mapper;
+
+import java.util.List;
+
+import org.apache.ibatis.annotations.Select;
+
+import org.apache.celeborn.server.common.service.model.ClusterSystemConfig;
+
+public interface ClusterSystemConfigMapper {
+
+ @Select(
+ "SELECT id, cluster_id, config_key, config_value, type, gmt_create, gmt_modify "
+ + "from celeborn_cluster_system_config where cluster_id = #{clusterId}")
Review Comment:
```suggestion
+ "FROM celeborn_cluster_system_config WHERE cluster_id = #{clusterId}")
```
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475565120
##########
service/src/main/resources/mybatis-config.xml:
##########
@@ -0,0 +1,43 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<!--
+ ~ 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.
+ -->
+
+<!DOCTYPE configuration
+ PUBLIC "-//mybatis.org//DTD Config 3.0//EN"
+ "http://mybatis.org/dtd/mybatis-3-config.dtd">
+<configuration>
+ <settings>
+ <setting name="mapUnderscoreToCamelCase" value="true" />
+ </settings>
+ <environments default="development">
+ <environment id="development">
+ <transactionManager type="JDBC"/>
+ <dataSource type="org.apache.celeborn.server.common.service.store.db.HikariDataSourceFactory">
Review Comment:
MyBatis seems to support Java-based configuration too?
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1473771020
##########
service/src/main/java/org/apache/celeborn/server/common/service/store/IServiceManager.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * 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.server.common.service.store;
+
+import java.util.List;
+
+import org.apache.celeborn.server.common.service.config.SystemConfig;
+import org.apache.celeborn.server.common.service.config.TenantConfig;
+import org.apache.celeborn.server.common.service.model.ClusterInfo;
+
+public interface IServiceManager {
+
+ int createClusterIfNotExists(ClusterInfo clusterInfo);
Review Comment:
```suggestion
int createCluster(ClusterInfo clusterInfo);
```
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477184890
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4376,6 +4395,78 @@ object CelebornConf extends Logging {
.timeConf(TimeUnit.MILLISECONDS)
.createWithDefaultString("120s")
+ val DYNAMIC_CONFIG_STORE_DB_DRIVER_CLASS_NAME: ConfigEntry[String] =
+ buildConf("celeborn.dynamicConfig.store.db.driverClassName")
+ .categories("master", "worker")
+ .version("0.5.0")
+ .doc("The jdbc driver class name of db store backend.")
+ .stringConf
+ .createWithDefaultString("")
+
+ val DYNAMIC_CONFIG_STORE_DB_JDBC_URL: ConfigEntry[String] =
+ buildConf("celeborn.dynamicConfig.store.db.jdbcUrl")
+ .categories("master", "worker")
+ .version("0.5.0")
+ .doc("The jdbc url of db store backend.")
+ .stringConf
+ .createWithDefaultString("")
+
+ val DYNAMIC_CONFIG_STORE_DB_USERNAME: ConfigEntry[String] =
+ buildConf("celeborn.dynamicConfig.store.db.username")
+ .categories("master", "worker")
+ .version("0.5.0")
+ .doc("The username of db store backend.")
+ .stringConf
+ .createWithDefaultString("")
+
+ val DYNAMIC_CONFIG_STORE_DB_PASSWORD: ConfigEntry[String] =
+ buildConf("celeborn.dynamicConfig.store.db.password")
+ .categories("master", "worker")
+ .version("0.5.0")
+ .doc("The password of db store backend.")
+ .stringConf
+ .createWithDefaultString("")
+
+ val DYNAMIC_CONFIG_STORE_DB_CONNECTION_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.dynamicConfig.store.db.connectionTimeout")
+ .categories("master", "worker")
+ .version("0.5.0")
+ .doc("The connection timeout that a client will wait for a connection from the pool for db store backend.")
+ .timeConf(TimeUnit.MILLISECONDS)
+ .createWithDefaultString("30s")
+
+ val DYNAMIC_CONFIG_STORE_DB_IDLE_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.dynamicConfig.store.db.idleTimeout")
Review Comment:
I mean passing ALL configurations starting with a prefix to the underlying Hikari pool, then we don't need to listing all keys in CelebornConf
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477249436
##########
service/src/test/resources/celeborn-0.5.0-h2.sql:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.
+ */
+
+CREATE SCHEMA IF NOT EXISTS TEST;
+
+SET SCHEMA TEST;
+DROP TABLE IF exists celeborn_cluster_info;
+DROP TABLE IF exists celeborn_cluster_system_config;
+DROP TABLE IF exists celeborn_cluster_tenant_config;
+
+CREATE TABLE IF NOT EXISTS celeborn_cluster_info
+(
+ id int NOT NULL AUTO_INCREMENT,
+ name varchar(255) NOT NULL COMMENT 'celeborn cluster name',
+ namespace varchar(255) DEFAULT NULL COMMENT 'celeborn cluster namespace',
+ endpoint varchar(255) DEFAULT NULL COMMENT 'celeborn cluster endpoint',
+ gmt_create timestamp NOT NULL,
+ gmt_modify timestamp NOT NULL,
+ PRIMARY KEY (id),
+ UNIQUE KEY `index_cluster_unique_name` (`name`)
+);
+
+CREATE TABLE IF NOT EXISTS celeborn_cluster_system_config
+(
+ id int NOT NULL AUTO_INCREMENT,
+ cluster_id int NOT NULL,
+ config_key varchar(255) NOT NULL,
+ config_value varchar(255) NOT NULL,
+ type varchar(255) DEFAULT NULL COMMENT 'conf categories, such as quota',
+ gmt_create timestamp NOT NULL,
+ gmt_modify timestamp NOT NULL,
+ PRIMARY KEY (id),
+ UNIQUE KEY `index_unique_system_config_key` (`cluster_id`, `config_key`)
+);
+
+CREATE TABLE IF NOT EXISTS celeborn_cluster_tenant_config
+(
+ id int NOT NULL AUTO_INCREMENT,
+ cluster_id int NOT NULL,
+ tenant_id varchar(255) NOT NULL,
+ level varchar(255) NOT NULL COMMENT 'config level, valid level is TENANT,USER',
+ `user` varchar(255) DEFAULT NULL COMMENT 'tenant sub user',
+ config_key varchar(255) NOT NULL,
+ config_value varchar(255) NOT NULL,
+ type varchar(255) DEFAULT NULL COMMENT 'conf categories, such as quota',
+ gmt_create timestamp NOT NULL,
+ gmt_modify timestamp NOT NULL,
+ PRIMARY KEY (id),
+ UNIQUE KEY `index_unique_tenant_config_key` (`cluster_id`, `tenant_id`, `user`, `config_key`)
+);
Review Comment:
nit: new line at the end
##########
service/src/test/resources/celeborn-0.5.0-h2-ut-data.sql:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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` )
+VALUES
+ ( 1, 'default', 'celeborn-1', 'celeborn-namespace.endpoint.com', '2023-08-26 22:08:30', '2023-08-26 22:08:30' );
+INSERT INTO `celeborn_cluster_system_config` ( `id`, `cluster_id`, `config_key`, `config_value`, `type`, `gmt_create`, `gmt_modify` )
+VALUES
+ ( 1, 1, 'celeborn.client.push.buffer.initial.size', '102400', 'QUOTA', '2023-08-26 22:08:30', '2023-08-26 22:08:30' );
+INSERT INTO `celeborn_cluster_system_config` ( `id`, `cluster_id`, `config_key`, `config_value`, `type`, `gmt_create`, `gmt_modify` )
+VALUES
+ ( 2, 1, 'celeborn.client.push.buffer.max.size', '1024000', 'QUOTA', '2023-08-26 22:08:30', '2023-08-26 22:08:30' );
+INSERT INTO `celeborn_cluster_system_config` ( `id`, `cluster_id`, `config_key`, `config_value`, `type`, `gmt_create`, `gmt_modify` )
+VALUES
+ ( 3, 1, 'celeborn.worker.fetch.heartbeat.enabled', 'true', 'QUOTA', '2023-08-26 22:08:30', '2023-08-26 22:08:30' );
+INSERT INTO `celeborn_cluster_system_config` ( `id`, `cluster_id`, `config_key`, `config_value`, `type`, `gmt_create`, `gmt_modify` )
+VALUES
+ ( 5, 1, 'celeborn.client.push.buffer.initial.size.only', '10240', 'QUOTA', '2023-08-26 22:08:30', '2023-08-26 22:08:30' );
+INSERT INTO `celeborn_cluster_system_config` ( `id`, `cluster_id`, `config_key`, `config_value`, `type`, `gmt_create`, `gmt_modify` )
+VALUES
+ ( 6, 1, 'celeborn.test.timeoutMs.only', '100s', 'QUOTA', '2023-08-26 22:08:30', '2023-08-26 22:08:30' );
+INSERT INTO `celeborn_cluster_system_config` ( `id`, `cluster_id`, `config_key`, `config_value`, `type`, `gmt_create`, `gmt_modify` )
+VALUES
+ ( 7, 1, 'celeborn.test.enabled.only', 'false', 'QUOTA', '2023-08-26 22:08:30', '2023-08-26 22:08:30' );
+INSERT INTO `celeborn_cluster_system_config` ( `id`, `cluster_id`, `config_key`, `config_value`, `type`, `gmt_create`, `gmt_modify` )
+VALUES
+ ( 8, 1, 'celeborn.test.int.only', '10', 'QUOTA', '2023-08-26 22:08:30', '2023-08-26 22:08:30' );
+INSERT INTO `celeborn_cluster_tenant_config` ( `id`, `cluster_id`, `tenant_id`, `level`, `user`, `config_key`, `config_value`, `type`, `gmt_create`, `gmt_modify` )
+VALUES
+ ( 1, 1, 'tenant_id', 'TENANT', '', 'celeborn.client.push.buffer.initial.size', '10240', 'QUOTA', '2023-08-26 22:08:30', '2023-08-26 22:08:30' );
+INSERT INTO `celeborn_cluster_tenant_config` ( `id`, `cluster_id`, `tenant_id`, `level`, `user`, `config_key`, `config_value`, `type`, `gmt_create`, `gmt_modify` )
+VALUES
+ ( 2, 1, 'tenant_id', 'TENANT', '', 'celeborn.client.push.buffer.initial.size.only', '102400', 'QUOTA', '2023-08-26 22:08:30', '2023-08-26 22:08:30' );
+INSERT INTO `celeborn_cluster_tenant_config` ( `id`, `cluster_id`, `tenant_id`, `level`, `user`, `config_key`, `config_value`, `type`, `gmt_create`, `gmt_modify` )
+VALUES
+ ( 3, 1, 'tenant_id', 'TENANT', '', 'celeborn.worker.fetch.heartbeat.enabled', 'false', 'QUOTA', '2023-08-26 22:08:30', '2023-08-26 22:08:30' );
+INSERT INTO `celeborn_cluster_tenant_config` ( `id`, `cluster_id`, `tenant_id`, `level`, `user`, `config_key`, `config_value`, `type`, `gmt_create`, `gmt_modify` )
+VALUES
+ ( 4, 1, 'tenant_id', 'TENANT', '', 'celeborn.test.tenant.timeoutMs.only', '100s', 'QUOTA', '2023-08-26 22:08:30', '2023-08-26 22:08:30' );
+INSERT INTO `celeborn_cluster_tenant_config` ( `id`, `cluster_id`, `tenant_id`, `level`, `user`, `config_key`, `config_value`, `type`, `gmt_create`, `gmt_modify` )
+VALUES
+ ( 5, 1, 'tenant_id', 'TENANT', '', 'celeborn.test.tenant.enabled.only', 'false', 'QUOTA', '2023-08-26 22:08:30', '2023-08-26 22:08:30' );
+INSERT INTO `celeborn_cluster_tenant_config` ( `id`, `cluster_id`, `tenant_id`, `level`, `user`, `config_key`, `config_value`, `type`, `gmt_create`, `gmt_modify` )
+VALUES
+ ( 6, 1, 'tenant_id', 'TENANT', '', 'celeborn.test.tenant.int.only', '100s', 'QUOTA', '2023-08-26 22:08:30', '2023-08-26 22:08:30' );
Review Comment:
nit: new line at the end
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477186870
##########
conf/mybatis-config.xml.template:
##########
@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<!--
+ ~ 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.
+ -->
+
+<!DOCTYPE configuration
+ PUBLIC "-//mybatis.org//DTD Config 3.0//EN"
+ "http://mybatis.org/dtd/mybatis-3-config.dtd">
+<configuration>
+ <settings>
+ <setting name="mapUnderscoreToCamelCase" value="true" />
+ </settings>
+ <environments default="development">
+ <environment id="development">
Review Comment:
According to https://mybatis.org/mybatis-3/java-api.html#sqlsessionfactorybuilder
If we choose `SqlSessionFactory build(Configuration config)`, we can avoid introducing the `mybatis-config.xml`
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477186056
##########
conf/mybatis-config.xml.template:
##########
@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<!--
+ ~ 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.
+ -->
+
+<!DOCTYPE configuration
+ PUBLIC "-//mybatis.org//DTD Config 3.0//EN"
+ "http://mybatis.org/dtd/mybatis-3-config.dtd">
+<configuration>
+ <settings>
+ <setting name="mapUnderscoreToCamelCase" value="true" />
+ </settings>
+ <environments default="development">
+ <environment id="development">
Review Comment:
Is environment block enforced? at least "development" is not a good name to me.
And, is it necessary to expose this file to end-users? I think we don't suppose they should modify it.
##########
conf/mybatis-config.xml.template:
##########
@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<!--
+ ~ 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.
+ -->
+
+<!DOCTYPE configuration
+ PUBLIC "-//mybatis.org//DTD Config 3.0//EN"
+ "http://mybatis.org/dtd/mybatis-3-config.dtd">
+<configuration>
+ <settings>
+ <setting name="mapUnderscoreToCamelCase" value="true" />
+ </settings>
+ <environments default="development">
+ <environment id="development">
Review Comment:
Is `environment` block enforced? at least "development" is not a good name to me.
And, is it necessary to expose this file to end-users? I think we don't suppose they should modify it.
--
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-1045] Support db based dynamic config server [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1473771020
##########
service/src/main/java/org/apache/celeborn/server/common/service/store/IServiceManager.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * 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.server.common.service.store;
+
+import java.util.List;
+
+import org.apache.celeborn.server.common.service.config.SystemConfig;
+import org.apache.celeborn.server.common.service.config.TenantConfig;
+import org.apache.celeborn.server.common.service.model.ClusterInfo;
+
+public interface IServiceManager {
+
+ int createClusterIfNotExists(ClusterInfo clusterInfo);
Review Comment:
```suggestion
int createCluster(ClusterInfo clusterInfo, boolean ignoreExists);
```
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4356,11 +4373,12 @@ object CelebornConf extends Logging {
val DYNAMIC_CONFIG_STORE_BACKEND: ConfigEntry[String] =
buildConf("celeborn.dynamicConfig.store.backend")
.categories("master", "worker")
- .doc("Store backend for dynamic config. Available options: NONE, FS. Note: NONE means disabling dynamic config store.")
+ .doc("Store backend for dynamic config service. Available options: NONE, FS, DB. Note: NONE means disabling " +
Review Comment:
Could we describe mort details for `FS` and `DB` to tell users which options should configure after the `celeborn.dynamicConfig.store.backend` is `FS` or `DB`?
##########
service/src/main/java/org/apache/celeborn/server/common/service/utils/JSON.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.server.common.service.utils;
+
+import com.fasterxml.jackson.annotation.JsonAutoDetect;
+import com.fasterxml.jackson.annotation.PropertyAccessor;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.commons.lang3.StringUtils;
+
+public class JSON {
Review Comment:
```suggestion
public class JsonUtils {
```
##########
service/src/main/java/org/apache/celeborn/server/common/service/config/ConfigService.java:
##########
@@ -17,22 +17,28 @@
package org.apache.celeborn.server.common.service.config;
+import java.io.IOException;
+
+import org.apache.celeborn.common.CelebornConf;
+
public interface ConfigService {
- SystemConfig getSystemConfig();
+ CelebornConf getCelebornConf();
+
+ SystemConfig getSystemConfigFromCache();
- TenantConfig getRawTenantConfig(String tenantId);
+ TenantConfig getRawTenantConfigFromCache(String tenantId);
- default DynamicConfig getTenantConfig(String tenantId) {
- TenantConfig tenantConfig = getRawTenantConfig(tenantId);
+ default DynamicConfig getTenantConfigFromCache(String tenantId) {
+ TenantConfig tenantConfig = getRawTenantConfigFromCache(tenantId);
if (tenantConfig == null || tenantConfig.getConfigs().isEmpty()) {
- return getSystemConfig();
+ return getSystemConfigFromCache();
} else {
return tenantConfig;
}
}
- void refreshAllCache();
+ void refreshAllCache() throws IOException;
Review Comment:
```suggestion
void refreshAllCache() throws Exception;
```
##########
service/src/main/java/org/apache/celeborn/server/common/service/config/DbConfigServiceImpl.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.server.common.service.config;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+
+import org.apache.celeborn.common.CelebornConf;
+import org.apache.celeborn.server.common.service.store.IServiceManager;
+import org.apache.celeborn.server.common.service.store.db.DbServiceManagerImpl;
+
+public class DbConfigServiceImpl extends BaseConfigServiceImpl implements ConfigService {
+ private volatile IServiceManager iServiceManager;
+
+ public DbConfigServiceImpl(CelebornConf celebornConf) throws IOException {
+ super(celebornConf);
Review Comment:
Could we create JDBC connection in constuctor? When the connection could not be created, it is unnecessary to refresh cache.
##########
service/src/main/java/org/apache/celeborn/server/common/service/store/db/DbServiceManagerImpl.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.server.common.service.store.db;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+import org.apache.ibatis.session.SqlSession;
+import org.apache.ibatis.session.SqlSessionFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.CelebornConf;
+import org.apache.celeborn.server.common.service.config.ConfigLevel;
+import org.apache.celeborn.server.common.service.config.ConfigService;
+import org.apache.celeborn.server.common.service.config.SystemConfig;
+import org.apache.celeborn.server.common.service.config.TenantConfig;
+import org.apache.celeborn.server.common.service.model.ClusterInfo;
+import org.apache.celeborn.server.common.service.model.ClusterSystemConfig;
+import org.apache.celeborn.server.common.service.model.ClusterTenantConfig;
+import org.apache.celeborn.server.common.service.store.IServiceManager;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterInfoMapper;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterSystemConfigMapper;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterTenantConfigMapper;
+import org.apache.celeborn.server.common.service.utils.JSON;
+
+public class DbServiceManagerImpl implements IServiceManager {
+ private static final Logger LOG = LoggerFactory.getLogger(DbServiceManagerImpl.class);
+ private final CelebornConf celebornConf;
+ private final ConfigService configService;
+ private final SqlSessionFactory sqlSessionFactory;
+ private final int clusterId;
+ private final int pageSize;
+
+ public DbServiceManagerImpl(CelebornConf celebornConf, ConfigService configServer)
+ throws IOException {
+ this.celebornConf = celebornConf;
+ this.sqlSessionFactory = DBSessionFactory.get(celebornConf);
+ this.configService = configServer;
+ this.pageSize = celebornConf.dynamicConfigStoreDbFetchPageSize();
+ this.clusterId = createClusterIfNotExists(getClusterInfoFromConf());
+ }
+
+ @Override
+ public int createClusterIfNotExists(ClusterInfo clusterInfo) {
+ ClusterInfo clusterInfoFromDB = getClusterInfo(clusterInfo.getName());
+ if (clusterInfoFromDB == null) {
+ try (SqlSession sqlSession = sqlSessionFactory.openSession(true)) {
+ ClusterInfoMapper mapper = sqlSession.getMapper(ClusterInfoMapper.class);
+ clusterInfo.setGmtCreate(new Date());
+ clusterInfo.setGmtModify(new Date());
+ try {
+ LOG.info("Cluster auto initialize with: {}", JSON.toJson(clusterInfo));
+ mapper.insert(clusterInfo);
+ } catch (Exception e) {
+ LOG.warn("Create Cluster error message: {}", e.getMessage(), e);
Review Comment:
```suggestion
LOG.warn("Create cluster failed: {}", e.getMessage(), e);
```
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475697524
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4356,11 +4373,12 @@ object CelebornConf extends Logging {
val DYNAMIC_CONFIG_STORE_BACKEND: ConfigEntry[String] =
buildConf("celeborn.dynamicConfig.store.backend")
.categories("master", "worker")
- .doc("Store backend for dynamic config. Available options: NONE, FS. Note: NONE means disabling dynamic config store.")
+ .doc("Store backend for dynamic config service. Available options: NONE, FS, DB. Note: NONE means disabling " +
Review Comment:
> Could we describe mort details for `FS` and `DB` to tell users which options should configure after the `celeborn.dynamicConfig.store.backend` is `FS` or `DB`?
I will add a doc for this feature later.
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475868332
##########
service/src/main/java/org/apache/celeborn/server/common/service/store/db/mapper/ClusterTenantConfigMapper.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.server.common.service.store.db.mapper;
+
+import java.util.List;
+
+import org.apache.ibatis.annotations.Param;
+import org.apache.ibatis.annotations.Select;
+
+import org.apache.celeborn.server.common.service.model.ClusterTenantConfig;
+
+public interface ClusterTenantConfigMapper {
+
+ @Select(
+ "SELECT id, cluster_id, tenant_id, level, user, config_key, config_value, type, gmt_create, gmt_modify "
+ + "from celeborn_cluster_tenant_config where cluster_id = #{clusterId} and level=#{level} limit #{offset}, #{pageSize}")
+ List<ClusterTenantConfig> getClusterTenantConfigs(
+ @Param("clusterId") int clusterId,
+ @Param("level") String configLevel,
+ @Param("offset") int offset,
+ @Param("pageSize") int pageSize);
+
+ @Select(
+ "SELECT count(*) from celeborn_cluster_tenant_config where cluster_id = #{clusterId} and level=#{level}")
Review Comment:
```suggestion
"SELECT count(*) FROM celeborn_cluster_tenant_config WHERE cluster_id = #{clusterId} AND level=#{level}")
```
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475869108
##########
service/src/main/java/org/apache/celeborn/server/common/service/store/db/mapper/ClusterTenantConfigMapper.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.server.common.service.store.db.mapper;
+
+import java.util.List;
+
+import org.apache.ibatis.annotations.Param;
+import org.apache.ibatis.annotations.Select;
+
+import org.apache.celeborn.server.common.service.model.ClusterTenantConfig;
+
+public interface ClusterTenantConfigMapper {
+
+ @Select(
+ "SELECT id, cluster_id, tenant_id, level, user, config_key, config_value, type, gmt_create, gmt_modify "
+ + "from celeborn_cluster_tenant_config where cluster_id = #{clusterId} and level=#{level} limit #{offset}, #{pageSize}")
Review Comment:
TODO: dialect should be considered, e.g. PG dialect
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477210400
##########
service/src/test/resources/celeborn-0.5.0-h2.sql:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.
+ */
+
+CREATE SCHEMA IF NOT EXISTS TEST;
+
+SET SCHEMA TEST;
+DROP TABLE IF exists celeborn_cluster_info;
+DROP TABLE IF exists celeborn_cluster_system_config;
+DROP TABLE IF exists celeborn_cluster_tenant_config;
+
+CREATE TABLE celeborn_cluster_info
+(
+ id int NOT NULL AUTO_INCREMENT,
+ name varchar(255) NOT NULL COMMENT 'celeborn cluster name',
+ namespace varchar(255) DEFAULT NULL COMMENT 'celeborn cluster namespace',
+ endpoint varchar(255) DEFAULT NULL COMMENT 'celeborn cluster endpoint',
+ gmt_create timestamp NULL DEFAULT NULL,
+ gmt_modify timestamp NULL DEFAULT NULL,
+ PRIMARY KEY (id),
+ UNIQUE KEY `index_cluster_unique_name` (`name`)
+ );
+
+CREATE TABLE celeborn_cluster_system_config
+(
+ id int NOT NULL AUTO_INCREMENT,
+ cluster_id int NOT NULL,
+ config_key varchar(255) DEFAULT NULL,
+ config_value varchar(255) DEFAULT NULL,
+ type varchar(255) DEFAULT NULL COMMENT 'conf categories, such as quota',
+ gmt_create timestamp NOT NULL,
+ gmt_modify timestamp NOT NULL,
+ PRIMARY KEY (id),
+ UNIQUE KEY `index_unique_system_config_key` (`cluster_id`, `config_key`)
+ );
+
+CREATE TABLE celeborn_cluster_tenant_config
+(
+ id int NOT NULL AUTO_INCREMENT,
+ cluster_id int NOT NULL,
+ tenant_id varchar(255) NOT NULL,
+ level varchar(255) NOT NULL COMMENT 'config level, valid level is TENANT,USER',
+ `user` varchar(255) DEFAULT NULL COMMENT 'tenant sub user',
+ config_key varchar(255) NOT NULL,
+ config_value varchar(255) NOT NULL,
+ type varchar(255) DEFAULT NULL COMMENT 'conf categories, such as quota',
+ gmt_create timestamp NOT NULL,
+ gmt_modify timestamp NOT NULL,
+ PRIMARY KEY (id),
+ UNIQUE KEY `index_unique_tenant_config_key` (`cluster_id`, `tenant_id`, `user`, `config_key`)
+ );
+
+
+INSERT INTO celeborn_cluster_info (`id`, `name`, `namespace`, `endpoint`, `gmt_create`, `gmt_modify`)
+VALUES (1, 'default', 'celeborn-1', 'celeborn-namespace.endpoint.com', '2023-08-26 22:08:30', '2023-08-26 22:08:30');
+
+
+INSERT INTO `celeborn_cluster_system_config` (`id`, `cluster_id`, `config_key`, `config_value`, `type`, `gmt_create`,
+ `gmt_modify`)
+VALUES (1, 1, 'celeborn.client.push.buffer.initial.size', '102400', 'QUOTA', '2023-08-26 22:08:30', '2023-08-26 22:08:30');
+INSERT INTO `celeborn_cluster_system_config` (`id`, `cluster_id`, `config_key`, `config_value`, `type`, `gmt_create`,
+ `gmt_modify`)
+VALUES (2, 1, 'celeborn.client.push.buffer.max.size', '1024000', 'QUOTA', '2023-08-26 22:08:30', '2023-08-26 22:08:30');
Review Comment:
can we format the data pretty? for example
```
INSERT INTO `celeborn_cluster_system_config`
(`id`, `cluster_id`, `config_key`, `config_value`, `type`, `gmt_create`, `gmt_modify`)
VALUES (1, 1, 'celeborn.client.push.buffer.initial.size', '102400', 'QUOTA', '2023-08-26 22:08:30', '2023-08-26 22:08:30');
```
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475778026
##########
service/src/main/resources/mybatis-config.xml:
##########
@@ -0,0 +1,43 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<!--
+ ~ 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.
+ -->
+
+<!DOCTYPE configuration
+ PUBLIC "-//mybatis.org//DTD Config 3.0//EN"
+ "http://mybatis.org/dtd/mybatis-3-config.dtd">
+<configuration>
+ <settings>
+ <setting name="mapUnderscoreToCamelCase" value="true" />
+ </settings>
+ <environments default="development">
+ <environment id="development">
+ <transactionManager type="JDBC"/>
+ <dataSource type="org.apache.celeborn.server.common.service.store.db.HikariDataSourceFactory">
Review Comment:
I mean avoid external configuration files as much as possible
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477185496
##########
toolkit/db/mysql/celeborn-0.5.0-mysql.sql:
##########
@@ -0,0 +1,56 @@
+/*
Review Comment:
the SQL still not is included in the final binary tgz, right?
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477207093
##########
service/src/main/resources/sql/mysql/celeborn-0.5.0-mysql.sql:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.
+ */
+CREATE TABLE IF NOT EXISTS celeborn_cluster_info
+(
+ id int NOT NULL AUTO_INCREMENT,
+ name varchar(255) NOT NULL COMMENT 'celeborn cluster name',
+ namespace varchar(255) DEFAULT NULL COMMENT 'celeborn cluster namespace',
+ endpoint varchar(255) DEFAULT NULL COMMENT 'celeborn cluster endpoint',
+ gmt_create timestamp NOT NULL,
+ gmt_modify timestamp NOT NULL,
+ PRIMARY KEY (id),
+ UNIQUE KEY `index_cluster_unique_name` (`name`)
+);
+
+CREATE TABLE IF NOT EXISTS celeborn_cluster_system_config
+(
+ id int NOT NULL AUTO_INCREMENT,
+ cluster_id int NOT NULL,
+ config_key varchar(255) NOT NULL,
+ config_value varchar(255) NOT NULL,
+ type varchar(255) DEFAULT NULL COMMENT 'conf categories, such as quota',
+ gmt_create timestamp NOT NULL,
+ gmt_modify timestamp NOT NULL,
+ PRIMARY KEY (id),
+ UNIQUE KEY `index_unique_system_config_key` (`cluster_id`, `config_key`)
+);
+
+CREATE TABLE IF NOT EXISTS celeborn_cluster_tenant_config
+(
+ id int NOT NULL AUTO_INCREMENT,
Review Comment:
nit: indent
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477249313
##########
service/src/test/resources/celeborn-0.5.0-h2.sql:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.
+ */
+
+CREATE SCHEMA IF NOT EXISTS TEST;
+
+SET SCHEMA TEST;
+DROP TABLE IF exists celeborn_cluster_info;
+DROP TABLE IF exists celeborn_cluster_system_config;
+DROP TABLE IF exists celeborn_cluster_tenant_config;
+
+CREATE TABLE IF NOT EXISTS celeborn_cluster_info
+(
+ id int NOT NULL AUTO_INCREMENT,
+ name varchar(255) NOT NULL COMMENT 'celeborn cluster name',
+ namespace varchar(255) DEFAULT NULL COMMENT 'celeborn cluster namespace',
+ endpoint varchar(255) DEFAULT NULL COMMENT 'celeborn cluster endpoint',
+ gmt_create timestamp NOT NULL,
+ gmt_modify timestamp NOT NULL,
+ PRIMARY KEY (id),
+ UNIQUE KEY `index_cluster_unique_name` (`name`)
+);
+
+CREATE TABLE IF NOT EXISTS celeborn_cluster_system_config
+(
+ id int NOT NULL AUTO_INCREMENT,
+ cluster_id int NOT NULL,
+ config_key varchar(255) NOT NULL,
+ config_value varchar(255) NOT NULL,
+ type varchar(255) DEFAULT NULL COMMENT 'conf categories, such as quota',
+ gmt_create timestamp NOT NULL,
+ gmt_modify timestamp NOT NULL,
+ PRIMARY KEY (id),
+ UNIQUE KEY `index_unique_system_config_key` (`cluster_id`, `config_key`)
+);
+
+CREATE TABLE IF NOT EXISTS celeborn_cluster_tenant_config
+(
+ id int NOT NULL AUTO_INCREMENT,
+ cluster_id int NOT NULL,
+ tenant_id varchar(255) NOT NULL,
+ level varchar(255) NOT NULL COMMENT 'config level, valid level is TENANT,USER',
+ `user` varchar(255) DEFAULT NULL COMMENT 'tenant sub user',
Review Comment:
nit
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475778849
##########
service/src/main/resources/db.sql:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.
+ */
+CREATE TABLE IF NOT EXISTS celeborn_cluster_info
Review Comment:
the schema DDL usually should be put outside when user untar the tgz, rather than inside the jar
--
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-1045] Support db based dynamic config server [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1474236748
##########
service/src/main/java/org/apache/celeborn/server/common/service/config/BaseConfigServiceImpl.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.server.common.service.config;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
+
+import scala.concurrent.duration.Duration;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.CelebornConf;
+import org.apache.celeborn.common.util.ThreadUtils;
+
+public abstract class BaseConfigServiceImpl implements ConfigService {
+ private static final Logger LOG = LoggerFactory.getLogger(BaseConfigServiceImpl.class);
+
+ protected final CelebornConf celebornConf;
+ protected final AtomicReference<SystemConfig> systemConfigAtomicReference =
+ new AtomicReference<>();
+ protected final AtomicReference<Map<String, TenantConfig>> tenantConfigAtomicReference =
+ new AtomicReference<>(new HashMap<>());
+ protected final long dynamicConfigRefreshTime;
+
+ private final ScheduledExecutorService configRefreshService =
+ ThreadUtils.newDaemonSingleThreadScheduledExecutor("celeborn-config-refresher");
+
+ public BaseConfigServiceImpl(CelebornConf celebornConf) throws IOException {
+ this.celebornConf = celebornConf;
+ this.systemConfigAtomicReference.set(new SystemConfig(celebornConf));
+ this.refreshAllCache();
+ this.dynamicConfigRefreshTime = celebornConf.dynamicConfigRefreshInterval();
+ this.configRefreshService.scheduleWithFixedDelay(
+ () -> {
+ try {
+ refreshAllCache();
+ } catch (Throwable e) {
+ LOG.error("Refresh configuration encounter exception: {}", e.getMessage(), e);
+ }
+ },
+ dynamicConfigRefreshTime,
+ dynamicConfigRefreshTime,
+ TimeUnit.MILLISECONDS);
+ }
+
+ @Override
+ public CelebornConf getCelebornConf() {
+ return celebornConf;
+ }
+
+ @Override
+ public SystemConfig getSystemConfigFromCache() {
+ return systemConfigAtomicReference.get();
+ }
+
+ @Override
+ public TenantConfig getRawTenantConfigFromCache(String tenantId) {
+ return tenantConfigAtomicReference.get().get(tenantId);
+ }
+
+ @Override
+ public void shutdown() {
+ ThreadUtils.shutdown(configRefreshService, Duration.apply("800ms"));
Review Comment:
```suggestion
ThreadUtils.shutdown(configRefreshService);
```
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475832876
##########
service/src/main/resources/db.sql:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.
+ */
+CREATE TABLE IF NOT EXISTS celeborn_cluster_info
Review Comment:
Good idea!
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477208041
##########
service/src/test/resources/celeborn-0.5.0-h2.sql:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.
+ */
+
+CREATE SCHEMA IF NOT EXISTS TEST;
+
+SET SCHEMA TEST;
+DROP TABLE IF exists celeborn_cluster_info;
+DROP TABLE IF exists celeborn_cluster_system_config;
+DROP TABLE IF exists celeborn_cluster_tenant_config;
+
+CREATE TABLE celeborn_cluster_info
+(
+ id int NOT NULL AUTO_INCREMENT,
+ name varchar(255) NOT NULL COMMENT 'celeborn cluster name',
+ namespace varchar(255) DEFAULT NULL COMMENT 'celeborn cluster namespace',
+ endpoint varchar(255) DEFAULT NULL COMMENT 'celeborn cluster endpoint',
+ gmt_create timestamp NULL DEFAULT NULL,
+ gmt_modify timestamp NULL DEFAULT NULL,
+ PRIMARY KEY (id),
+ UNIQUE KEY `index_cluster_unique_name` (`name`)
+ );
+
+CREATE TABLE celeborn_cluster_system_config
+(
+ id int NOT NULL AUTO_INCREMENT,
+ cluster_id int NOT NULL,
+ config_key varchar(255) DEFAULT NULL,
Review Comment:
keep nullability consistent with MySQL DDL
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475689519
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -2201,6 +2210,14 @@ object CelebornConf extends Logging {
.timeConf(TimeUnit.MILLISECONDS)
.createWithDefaultString("30s")
+ val CLUSTER_NAME: ConfigEntry[String] =
+ buildConf("celeborn.cluster.name")
+ .categories("master", "worker")
+ .version("0.5.0")
+ .doc("celeborn cluster name")
Review Comment:
```suggestion
.doc("Celeborn cluster name.")
```
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477220374
##########
service/src/main/java/org/apache/celeborn/server/common/service/store/db/DBSessionFactory.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.server.common.service.store.db;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.Properties;
+
+import javax.sql.DataSource;
+
+import com.zaxxer.hikari.HikariConfig;
+import com.zaxxer.hikari.HikariDataSource;
+import org.apache.ibatis.mapping.Environment;
+import org.apache.ibatis.session.Configuration;
+import org.apache.ibatis.session.SqlSessionFactory;
+import org.apache.ibatis.session.SqlSessionFactoryBuilder;
+import org.apache.ibatis.transaction.TransactionFactory;
+import org.apache.ibatis.transaction.jdbc.JdbcTransactionFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.CelebornConf;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterInfoMapper;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterSystemConfigMapper;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterTenantConfigMapper;
+
+public class DBSessionFactory {
+ private static final Logger LOG = LoggerFactory.getLogger(DBSessionFactory.class);
+ private static volatile SqlSessionFactory _instance;
+
+ public static SqlSessionFactory get(CelebornConf celebornConf) throws IOException {
+ if (_instance == null) {
+ synchronized (DBSessionFactory.class) {
+ if (_instance == null) {
+ Properties properties = new Properties();
+ properties.setProperty(
+ "driverClassName", celebornConf.dynamicConfigStoreDbHikariDriverClassName());
+ properties.setProperty("jdbcUrl", celebornConf.dynamicConfigStoreDbHikariJdbcUrl());
+ properties.setProperty("username", celebornConf.dynamicConfigStoreDbHikariUsername());
+ properties.setProperty("password", celebornConf.dynamicConfigStoreDbHikariPassword());
+ properties.setProperty(
+ "connectionTimeout",
+ String.valueOf(celebornConf.dynamicConfigStoreDbHikariConnectionTimeout()));
+ properties.setProperty(
+ "idleTimeout", String.valueOf(celebornConf.dynamicConfigStoreDbHikariIdleTimeout()));
+ properties.setProperty(
+ "maxLifetime", String.valueOf(celebornConf.dynamicConfigStoreDbHikariMaxLifetime()));
+ properties.setProperty(
+ "maximumPoolSize",
+ String.valueOf(celebornConf.dynamicConfigStoreDbHikariMaximumPoolSize()));
+
+ for (Map.Entry<String, String> dbPropertiesEntry :
+ celebornConf.dynamicConfigStoreDbHikariCustomConfigs().entrySet()) {
+ properties.setProperty(
+ dbPropertiesEntry.getKey().replace("celeborn.dynamicConfig.store.db.hikari.", ""),
+ dbPropertiesEntry.getValue());
+ }
+
+ HikariConfig config = new HikariConfig(properties);
+ DataSource dataSource = new HikariDataSource(config);
+
+ TransactionFactory transactionFactory = new JdbcTransactionFactory();
+ Environment environment = new Environment("celeborn", transactionFactory, dataSource);
+
+ Configuration configuration = new Configuration(environment);
+ configuration.setMapUnderscoreToCamelCase(true);
Review Comment:
yes
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475562541
##########
service/src/main/java/org/apache/celeborn/server/common/service/config/DbConfigServiceImpl.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.server.common.service.config;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+
+import org.apache.celeborn.common.CelebornConf;
+import org.apache.celeborn.server.common.service.store.IServiceManager;
+import org.apache.celeborn.server.common.service.store.db.DbServiceManagerImpl;
+
+public class DbConfigServiceImpl extends BaseConfigServiceImpl implements ConfigService {
+ private volatile IServiceManager iServiceManager;
+
+ public DbConfigServiceImpl(CelebornConf celebornConf) throws IOException {
+ super(celebornConf);
Review Comment:
> Could we create JDBC connection in constuctor? When the connection could not be created, it is unnecessary to refresh cache.
You can refer the constructor of BaseConfigServiceImpl, when configService initialize, it will do a first refresh, if there has something wrong, server shouldn't be startup. Then it would refresh in a daemon thread.
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477203331
##########
NOTICE-binary:
##########
@@ -123,3 +123,15 @@ This software includes projects with other licenses -- see `doc/LICENSE.md`.
This product includes software developed by Google
Snappy: http://code.google.com/p/snappy/ (New BSD License)
+
+MyBatis
+Copyright 2010-2023
+
+This product includes software developed by
+The MyBatis Team (https://www.mybatis.org/).
+
+iBATIS
+ This product includes software developed by
+ The Apache Software Foundation (https://www.apache.org/).
+
+ Copyright 2010 The Apache Software Foundation
Review Comment:
For the iBATIS part, we can simplify it to
```
iBATIS
Copyright 2010 The Apache Software Foundation
```
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477217906
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -367,7 +367,22 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
}
def dynamicConfigStoreBackend: String = get(DYNAMIC_CONFIG_STORE_BACKEND)
- def dynamicConfigRefreshInterval: Long = get(DYNAMIC_CONFIG_REFRESH_INTERVAL)
+ def dynamicConfigRefreshInterval: Long = get(DYNAMIC_CONFIG_REFRESH_INTERVAL).getOrElse(-1L)
Review Comment:
why not just return a `Option[Long]`?
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477343605
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4361,20 +4385,93 @@ object CelebornConf extends Logging {
val DYNAMIC_CONFIG_STORE_BACKEND: ConfigEntry[String] =
buildConf("celeborn.dynamicConfig.store.backend")
.categories("master", "worker")
- .doc("Store backend for dynamic config. Available options: NONE, FS. Note: NONE means disabling dynamic config store.")
+ .doc("Store backend for dynamic config service. Available options: FS, DB.")
.version("0.4.0")
.stringConf
.transform(_.toUpperCase(Locale.ROOT))
- .checkValues(Set("NONE", "FS"))
- .createWithDefault("NONE")
+ .checkValues(Set("FS", "DB"))
+ .createWithDefault("FS")
- val DYNAMIC_CONFIG_REFRESH_INTERVAL: ConfigEntry[Long] =
+ val DYNAMIC_CONFIG_REFRESH_INTERVAL: OptionalConfigEntry[Long] =
buildConf("celeborn.dynamicConfig.refresh.interval")
Review Comment:
Make sense, I add a new configuration `celeborn.dynamicConfig.refresh.enabled` to switch, that's more clear now.
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475741578
##########
service/src/main/java/org/apache/celeborn/server/common/service/store/db/DbServiceManagerImpl.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * 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.server.common.service.store.db;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+import org.apache.ibatis.session.SqlSession;
+import org.apache.ibatis.session.SqlSessionFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.CelebornConf;
+import org.apache.celeborn.server.common.service.config.ConfigLevel;
+import org.apache.celeborn.server.common.service.config.ConfigService;
+import org.apache.celeborn.server.common.service.config.SystemConfig;
+import org.apache.celeborn.server.common.service.config.TenantConfig;
+import org.apache.celeborn.server.common.service.model.ClusterInfo;
+import org.apache.celeborn.server.common.service.model.ClusterSystemConfig;
+import org.apache.celeborn.server.common.service.model.ClusterTenantConfig;
+import org.apache.celeborn.server.common.service.store.IServiceManager;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterInfoMapper;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterSystemConfigMapper;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterTenantConfigMapper;
+import org.apache.celeborn.server.common.service.utils.JsonUtils;
+
+public class DbServiceManagerImpl implements IServiceManager {
+ private static final Logger LOG = LoggerFactory.getLogger(DbServiceManagerImpl.class);
+ private final CelebornConf celebornConf;
+ private final ConfigService configService;
+ private final SqlSessionFactory sqlSessionFactory;
+ private final int clusterId;
+ private final int pageSize;
+
+ public DbServiceManagerImpl(CelebornConf celebornConf, ConfigService configServer)
+ throws IOException {
+ this.celebornConf = celebornConf;
+ this.sqlSessionFactory = DBSessionFactory.get(celebornConf);
+ this.configService = configServer;
+ this.pageSize = celebornConf.dynamicConfigStoreDbFetchPageSize();
+ this.clusterId = createCluster(getClusterInfoFromConf());
+ }
+
+ @Override
+ public int createCluster(ClusterInfo clusterInfo) {
+ ClusterInfo clusterInfoFromDB = getClusterInfo(clusterInfo.getName());
+ if (clusterInfoFromDB == null) {
+ try (SqlSession sqlSession = sqlSessionFactory.openSession(true)) {
+ ClusterInfoMapper mapper = sqlSession.getMapper(ClusterInfoMapper.class);
+ clusterInfo.setGmtCreate(new Date());
+ clusterInfo.setGmtModify(new Date());
+ LOG.info("Cluster auto initialize with: {}", JsonUtils.toJson(clusterInfo));
+ mapper.insert(clusterInfo);
+ } catch (Exception e) {
+ LOG.warn("Create Cluster failed: {}", e.getMessage(), e);
Review Comment:
```suggestion
LOG.warn("Create cluster {} failed: {}.", JsonUtils.toJson(clusterInfo), e.getMessage(), e);
```
##########
service/src/main/java/org/apache/celeborn/server/common/service/store/db/DbServiceManagerImpl.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * 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.server.common.service.store.db;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+import org.apache.ibatis.session.SqlSession;
+import org.apache.ibatis.session.SqlSessionFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.CelebornConf;
+import org.apache.celeborn.server.common.service.config.ConfigLevel;
+import org.apache.celeborn.server.common.service.config.ConfigService;
+import org.apache.celeborn.server.common.service.config.SystemConfig;
+import org.apache.celeborn.server.common.service.config.TenantConfig;
+import org.apache.celeborn.server.common.service.model.ClusterInfo;
+import org.apache.celeborn.server.common.service.model.ClusterSystemConfig;
+import org.apache.celeborn.server.common.service.model.ClusterTenantConfig;
+import org.apache.celeborn.server.common.service.store.IServiceManager;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterInfoMapper;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterSystemConfigMapper;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterTenantConfigMapper;
+import org.apache.celeborn.server.common.service.utils.JsonUtils;
+
+public class DbServiceManagerImpl implements IServiceManager {
+ private static final Logger LOG = LoggerFactory.getLogger(DbServiceManagerImpl.class);
+ private final CelebornConf celebornConf;
+ private final ConfigService configService;
+ private final SqlSessionFactory sqlSessionFactory;
+ private final int clusterId;
+ private final int pageSize;
+
+ public DbServiceManagerImpl(CelebornConf celebornConf, ConfigService configServer)
+ throws IOException {
+ this.celebornConf = celebornConf;
+ this.sqlSessionFactory = DBSessionFactory.get(celebornConf);
+ this.configService = configServer;
+ this.pageSize = celebornConf.dynamicConfigStoreDbFetchPageSize();
+ this.clusterId = createCluster(getClusterInfoFromConf());
+ }
+
+ @Override
+ public int createCluster(ClusterInfo clusterInfo) {
+ ClusterInfo clusterInfoFromDB = getClusterInfo(clusterInfo.getName());
+ if (clusterInfoFromDB == null) {
+ try (SqlSession sqlSession = sqlSessionFactory.openSession(true)) {
+ ClusterInfoMapper mapper = sqlSession.getMapper(ClusterInfoMapper.class);
+ clusterInfo.setGmtCreate(new Date());
+ clusterInfo.setGmtModify(new Date());
+ LOG.info("Cluster auto initialize with: {}", JsonUtils.toJson(clusterInfo));
Review Comment:
```suggestion
LOG.info("Created cluster {} successfully.", JsonUtils.toJson(clusterInfo));
```
##########
service/src/main/java/org/apache/celeborn/server/common/service/store/db/DbServiceManagerImpl.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * 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.server.common.service.store.db;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+import org.apache.ibatis.session.SqlSession;
+import org.apache.ibatis.session.SqlSessionFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.CelebornConf;
+import org.apache.celeborn.server.common.service.config.ConfigLevel;
+import org.apache.celeborn.server.common.service.config.ConfigService;
+import org.apache.celeborn.server.common.service.config.SystemConfig;
+import org.apache.celeborn.server.common.service.config.TenantConfig;
+import org.apache.celeborn.server.common.service.model.ClusterInfo;
+import org.apache.celeborn.server.common.service.model.ClusterSystemConfig;
+import org.apache.celeborn.server.common.service.model.ClusterTenantConfig;
+import org.apache.celeborn.server.common.service.store.IServiceManager;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterInfoMapper;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterSystemConfigMapper;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterTenantConfigMapper;
+import org.apache.celeborn.server.common.service.utils.JsonUtils;
+
+public class DbServiceManagerImpl implements IServiceManager {
+ private static final Logger LOG = LoggerFactory.getLogger(DbServiceManagerImpl.class);
+ private final CelebornConf celebornConf;
+ private final ConfigService configService;
+ private final SqlSessionFactory sqlSessionFactory;
+ private final int clusterId;
+ private final int pageSize;
+
+ public DbServiceManagerImpl(CelebornConf celebornConf, ConfigService configServer)
+ throws IOException {
+ this.celebornConf = celebornConf;
+ this.sqlSessionFactory = DBSessionFactory.get(celebornConf);
+ this.configService = configServer;
+ this.pageSize = celebornConf.dynamicConfigStoreDbFetchPageSize();
+ this.clusterId = createCluster(getClusterInfoFromConf());
+ }
+
+ @Override
+ public int createCluster(ClusterInfo clusterInfo) {
+ ClusterInfo clusterInfoFromDB = getClusterInfo(clusterInfo.getName());
+ if (clusterInfoFromDB == null) {
+ try (SqlSession sqlSession = sqlSessionFactory.openSession(true)) {
+ ClusterInfoMapper mapper = sqlSession.getMapper(ClusterInfoMapper.class);
+ clusterInfo.setGmtCreate(new Date());
+ clusterInfo.setGmtModify(new Date());
+ LOG.info("Cluster auto initialize with: {}", JsonUtils.toJson(clusterInfo));
+ mapper.insert(clusterInfo);
+ } catch (Exception e) {
+ LOG.warn("Create Cluster failed: {}", e.getMessage(), e);
+ }
+ clusterInfoFromDB = getClusterInfo(clusterInfo.getName());
+ if (clusterInfoFromDB == null) {
+ throw new RuntimeException("Can't get cluster info.");
Review Comment:
```suggestion
throw new RuntimeException("Could not get cluster info of {}.", clusterInfo.getName());
```
##########
service/src/main/java/org/apache/celeborn/server/common/service/store/db/DbServiceManagerImpl.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * 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.server.common.service.store.db;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+import org.apache.ibatis.session.SqlSession;
+import org.apache.ibatis.session.SqlSessionFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.CelebornConf;
+import org.apache.celeborn.server.common.service.config.ConfigLevel;
+import org.apache.celeborn.server.common.service.config.ConfigService;
+import org.apache.celeborn.server.common.service.config.SystemConfig;
+import org.apache.celeborn.server.common.service.config.TenantConfig;
+import org.apache.celeborn.server.common.service.model.ClusterInfo;
+import org.apache.celeborn.server.common.service.model.ClusterSystemConfig;
+import org.apache.celeborn.server.common.service.model.ClusterTenantConfig;
+import org.apache.celeborn.server.common.service.store.IServiceManager;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterInfoMapper;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterSystemConfigMapper;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterTenantConfigMapper;
+import org.apache.celeborn.server.common.service.utils.JsonUtils;
+
+public class DbServiceManagerImpl implements IServiceManager {
+ private static final Logger LOG = LoggerFactory.getLogger(DbServiceManagerImpl.class);
+ private final CelebornConf celebornConf;
+ private final ConfigService configService;
+ private final SqlSessionFactory sqlSessionFactory;
+ private final int clusterId;
+ private final int pageSize;
+
+ public DbServiceManagerImpl(CelebornConf celebornConf, ConfigService configServer)
+ throws IOException {
+ this.celebornConf = celebornConf;
+ this.sqlSessionFactory = DBSessionFactory.get(celebornConf);
+ this.configService = configServer;
+ this.pageSize = celebornConf.dynamicConfigStoreDbFetchPageSize();
+ this.clusterId = createCluster(getClusterInfoFromConf());
+ }
+
+ @Override
+ public int createCluster(ClusterInfo clusterInfo) {
+ ClusterInfo clusterInfoFromDB = getClusterInfo(clusterInfo.getName());
+ if (clusterInfoFromDB == null) {
+ try (SqlSession sqlSession = sqlSessionFactory.openSession(true)) {
+ ClusterInfoMapper mapper = sqlSession.getMapper(ClusterInfoMapper.class);
+ clusterInfo.setGmtCreate(new Date());
+ clusterInfo.setGmtModify(new Date());
+ LOG.info("Cluster auto initialize with: {}", JsonUtils.toJson(clusterInfo));
+ mapper.insert(clusterInfo);
+ } catch (Exception e) {
+ LOG.warn("Create Cluster failed: {}", e.getMessage(), e);
+ }
+ clusterInfoFromDB = getClusterInfo(clusterInfo.getName());
+ if (clusterInfoFromDB == null) {
+ throw new RuntimeException("Can't get cluster info.");
+ }
+ }
+ return clusterInfoFromDB.getId();
+ }
+
+ @Override
+ public ClusterInfo getClusterInfo(String clusterName) {
+ try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
+ ClusterInfoMapper mapper = sqlSession.getMapper(ClusterInfoMapper.class);
+ return mapper.getClusterInfo(clusterName);
+ }
+ }
+
+ @Override
+ public List<TenantConfig> getAllTenantConfigs() {
+ try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
+ ClusterTenantConfigMapper mapper = sqlSession.getMapper(ClusterTenantConfigMapper.class);
+ int totalNum = mapper.getClusterTenantConfigsNum(clusterId, ConfigLevel.TENANT.name());
+ int offset = 0;
+ List<ClusterTenantConfig> clusterAllTenantConfigs = new ArrayList<>();
+ while (offset < totalNum) {
+ List<ClusterTenantConfig> clusterTenantConfigs =
+ mapper.getClusterTenantConfigs(clusterId, ConfigLevel.TENANT.name(), offset, pageSize);
+ clusterAllTenantConfigs.addAll(clusterTenantConfigs);
+ offset = offset + pageSize;
+ }
+
+ Map<String, List<ClusterTenantConfig>> tenantConfigMaps =
+ clusterAllTenantConfigs.stream()
+ .collect(
+ Collectors.groupingBy(clusterTenantConfig -> clusterTenantConfig.getTenantId()));
+ return tenantConfigMaps.entrySet().stream()
+ .map(t -> new TenantConfig(configService, t.getKey(), null, t.getValue()))
+ .collect(Collectors.toList());
+ }
+ }
+
+ @Override
+ public SystemConfig getSystemConfig() {
+ try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
+ ClusterSystemConfigMapper mapper = sqlSession.getMapper(ClusterSystemConfigMapper.class);
+ List<ClusterSystemConfig> clusterSystemConfig = mapper.getClusterSystemConfig(clusterId);
+ return new SystemConfig(celebornConf, clusterSystemConfig);
+ }
+ }
+
+ public ClusterInfo getClusterInfoFromConf() {
Review Comment:
```suggestion
public ClusterInfo getClusterInfoFromEnv() {
```
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475778849
##########
service/src/main/resources/db.sql:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.
+ */
+CREATE TABLE IF NOT EXISTS celeborn_cluster_info
Review Comment:
the schema DDL usually should be put outside when user untar the tgz, rather inside the jar
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475805725
##########
service/src/main/java/org/apache/celeborn/server/common/service/config/DbConfigServiceImpl.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.server.common.service.config;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+
+import org.apache.celeborn.common.CelebornConf;
+import org.apache.celeborn.server.common.service.store.IServiceManager;
+import org.apache.celeborn.server.common.service.store.db.DbServiceManagerImpl;
+
+public class DbConfigServiceImpl extends BaseConfigServiceImpl implements ConfigService {
+ private volatile IServiceManager iServiceManager;
+
+ public DbConfigServiceImpl(CelebornConf celebornConf) throws IOException {
+ super(celebornConf);
+ }
+
+ @Override
+ public void refreshAllCache() throws IOException {
+ if (iServiceManager == null) {
+ synchronized (this) {
+ iServiceManager = new DbServiceManagerImpl(celebornConf, this);
Review Comment:
check null 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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475785564
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4356,11 +4373,12 @@ object CelebornConf extends Logging {
val DYNAMIC_CONFIG_STORE_BACKEND: ConfigEntry[String] =
buildConf("celeborn.dynamicConfig.store.backend")
.categories("master", "worker")
- .doc("Store backend for dynamic config. Available options: NONE, FS. Note: NONE means disabling dynamic config store.")
+ .doc("Store backend for dynamic config service. Available options: NONE, FS, DB. Note: NONE means disabling " +
Review Comment:
IMO the current configurations may confusing users.
Both `NONE` and `FS` indicates reading conf from file, NONE does not support refresh but FS does. In such cases, would it be more clear to replace with the following configurations:
- celeborn.configuration.provider: <FILE|DB>
- celeborn.configuration.refresh.interval: <None means disable|Some(int) means enable>
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475867831
##########
service/src/main/java/org/apache/celeborn/server/common/service/store/db/mapper/ClusterTenantConfigMapper.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.server.common.service.store.db.mapper;
+
+import java.util.List;
+
+import org.apache.ibatis.annotations.Param;
+import org.apache.ibatis.annotations.Select;
+
+import org.apache.celeborn.server.common.service.model.ClusterTenantConfig;
+
+public interface ClusterTenantConfigMapper {
+
+ @Select(
+ "SELECT id, cluster_id, tenant_id, level, user, config_key, config_value, type, gmt_create, gmt_modify "
+ + "from celeborn_cluster_tenant_config where cluster_id = #{clusterId} and level=#{level} limit #{offset}, #{pageSize}")
Review Comment:
```suggestion
+ "FROM celeborn_cluster_tenant_config WHERE cluster_id = #{clusterId} AND level=#{level} LIMIT #{offset}, #{pageSize}")
```
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475799895
##########
service/src/main/resources/db.sql:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.
+ */
+CREATE TABLE IF NOT EXISTS celeborn_cluster_info
+(
+ id int NOT NULL AUTO_INCREMENT,
+ name varchar(255) NOT NULL COMMENT 'celeborn cluster name',
+ namespace varchar(255) DEFAULT NULL COMMENT 'celeborn cluster namespace',
+ endpoint varchar(255) DEFAULT NULL COMMENT 'celeborn cluster endpoint',
+ gmt_create timestamp NULL DEFAULT NULL,
+ gmt_modify timestamp NULL DEFAULT NULL,
+ PRIMARY KEY (id),
+ UNIQUE KEY `index_cluster_unique_name` (`name`)
+);
+
+CREATE TABLE IF NOT EXISTS celeborn_cluster_system_config
+(
+ id int NOT NULL AUTO_INCREMENT,
+ cluster_id int NOT NULL,
+ config_key varchar(255) DEFAULT NULL,
+ config_value varchar(255) DEFAULT NULL,
+ type varchar(255) DEFAULT NULL COMMENT 'conf categories, such as quota',
+ gmt_create timestamp NULL DEFAULT NULL,
+ gmt_modify timestamp NULL DEFAULT NULL,
+ PRIMARY KEY (id),
+ UNIQUE KEY `index_unique_system_config_key` (`cluster_id`, `config_key`)
+);
+
+CREATE TABLE IF NOT EXISTS celeborn_cluster_tenant_config
+(
+ id int NOT NULL AUTO_INCREMENT,
+ cluster_id int NOT NULL,
+ tenant_id varchar(255) NOT NULL,
+ level varchar(255) DEFAULT NULL COMMENT 'config level, valid level is TENANT、USER',
Review Comment:
`、` is a non-ascii char
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475796055
##########
service/src/main/resources/db.sql:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.
+ */
+CREATE TABLE IF NOT EXISTS celeborn_cluster_info
Review Comment:
And the file name should contains version and DB type, e.g. `celeborn-mysql-0.5.0.sql`
##########
service/src/main/resources/db.sql:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.
+ */
+CREATE TABLE IF NOT EXISTS celeborn_cluster_info
Review Comment:
And the file name should contain version and DB type, e.g. `celeborn-mysql-0.5.0.sql`
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475801574
##########
service/src/main/resources/db.sql:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.
+ */
+CREATE TABLE IF NOT EXISTS celeborn_cluster_info
+(
+ id int NOT NULL AUTO_INCREMENT,
+ name varchar(255) NOT NULL COMMENT 'celeborn cluster name',
+ namespace varchar(255) DEFAULT NULL COMMENT 'celeborn cluster namespace',
+ endpoint varchar(255) DEFAULT NULL COMMENT 'celeborn cluster endpoint',
+ gmt_create timestamp NULL DEFAULT NULL,
+ gmt_modify timestamp NULL DEFAULT NULL,
Review Comment:
Emm ... why should they NULLABLE
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475785564
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4356,11 +4373,12 @@ object CelebornConf extends Logging {
val DYNAMIC_CONFIG_STORE_BACKEND: ConfigEntry[String] =
buildConf("celeborn.dynamicConfig.store.backend")
.categories("master", "worker")
- .doc("Store backend for dynamic config. Available options: NONE, FS. Note: NONE means disabling dynamic config store.")
+ .doc("Store backend for dynamic config service. Available options: NONE, FS, DB. Note: NONE means disabling " +
Review Comment:
IMO the current configurations may confusing users.
Both `NONE` and `FS` indicates reading conf from file, NONE does not support refresh but FS does. In such cases, would it be more clear to replace it with the following configurations:
- celeborn.configuration.provider: <FILE|DB>
- celeborn.configuration.refresh.interval: <None means disable|Some(int) means enable>
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1476200510
##########
service/src/test/resources/db.sql:
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.
+ */
+
+CREATE SCHEMA IF NOT EXISTS TEST;
+
+SET SCHEMA TEST;
+DROP TABLE IF exists celeborn_cluster_info;
+DROP TABLE IF exists celeborn_cluster_system_config;
+DROP TABLE IF exists celeborn_cluster_tenant_config;
+
+CREATE TABLE celeborn_cluster_info
Review Comment:
> why write `CREATE TABLE` statements again? we should use the user-oriented DDL file
This file is for h2 database initialization in UT test
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477193930
##########
LICENSE-binary:
##########
@@ -251,6 +252,7 @@ org.apache.commons:commons-crypto
org.apache.commons:commons-lang3
org.apache.hadoop:hadoop-client-api
org.apache.hadoop:hadoop-client-runtime
+org.apache.ibatis:mybatis
Review Comment:
this is a NOTICE file inside mybatis jar, the NOTICE-binary should be updated correspondingly,
##########
LICENSE-binary:
##########
@@ -251,6 +252,7 @@ org.apache.commons:commons-crypto
org.apache.commons:commons-lang3
org.apache.hadoop:hadoop-client-api
org.apache.hadoop:hadoop-client-runtime
+org.apache.ibatis:mybatis
Review Comment:
this is a NOTICE file inside mybatis jar, the NOTICE-binary should be updated correspondingly
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477203490
##########
NOTICE:
##########
@@ -39,3 +39,15 @@ Copyright 2021 The Flink Remote Shuffle Project
This product includes software developed at The Alibaba Group Holding Ltd
(https://www.alibaba.com/).
+
+MyBatis
Review Comment:
only NOTICE-binary should be updated, because we don't copy the source code from MyBatis
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477203256
##########
NOTICE-binary:
##########
@@ -123,3 +123,15 @@ This software includes projects with other licenses -- see `doc/LICENSE.md`.
This product includes software developed by Google
Snappy: http://code.google.com/p/snappy/ (New BSD License)
+
+MyBatis
+Copyright 2010-2023
+
+This product includes software developed by
+The MyBatis Team (https://www.mybatis.org/).
+
+iBATIS
+ This product includes software developed by
+ The Apache Software Foundation (https://www.apache.org/).
+
+ Copyright 2010 The Apache Software Foundation
Review Comment:
the remainding content is required too, because those part is included in mybatis jar and shipped by our binary tgz
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477306828
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4361,20 +4385,93 @@ object CelebornConf extends Logging {
val DYNAMIC_CONFIG_STORE_BACKEND: ConfigEntry[String] =
buildConf("celeborn.dynamicConfig.store.backend")
.categories("master", "worker")
- .doc("Store backend for dynamic config. Available options: NONE, FS. Note: NONE means disabling dynamic config store.")
+ .doc("Store backend for dynamic config service. Available options: FS, DB.")
.version("0.4.0")
.stringConf
.transform(_.toUpperCase(Locale.ROOT))
- .checkValues(Set("NONE", "FS"))
- .createWithDefault("NONE")
+ .checkValues(Set("FS", "DB"))
+ .createWithDefault("FS")
- val DYNAMIC_CONFIG_REFRESH_INTERVAL: ConfigEntry[Long] =
+ val DYNAMIC_CONFIG_REFRESH_INTERVAL: OptionalConfigEntry[Long] =
buildConf("celeborn.dynamicConfig.refresh.interval")
Review Comment:
How does the user know that the dynamic config does not update well when `celeborn.dynamicConfig.refresh.interval` is not configured?
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475566611
##########
service/src/main/resources/mybatis-config.xml:
##########
@@ -0,0 +1,43 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<!--
+ ~ 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.
+ -->
+
+<!DOCTYPE configuration
+ PUBLIC "-//mybatis.org//DTD Config 3.0//EN"
+ "http://mybatis.org/dtd/mybatis-3-config.dtd">
+<configuration>
+ <settings>
+ <setting name="mapUnderscoreToCamelCase" value="true" />
+ </settings>
+ <environments default="development">
+ <environment id="development">
+ <transactionManager type="JDBC"/>
+ <dataSource type="org.apache.celeborn.server.common.service.store.db.HikariDataSourceFactory">
+ <property name="jdbcUrl" value="${jdbcUrl}"/>
+ <property name="username" value="${username}"/>
+ <property name="password" value="${password}"/>
+ <property name="connectionTimeout" value="${connectionTimeout}"/>
+ <property name="idleTimeout" value="${idleTimeout}"/>
+ <property name="maxLifetime" value="${maxLifetime}"/>
+ <property name="maximumPoolSize" value="${maximumPoolSize}"/>
+ </dataSource>
+ </environment>
+ </environments>
+ <mappers>
+ <package name="org.apache.celeborn.server.common.service.store.db.mapper"/>
Review Comment:
can we list all classes instead? searching in classpath with package name may consume certain memories, typically 500MiB+
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475866639
##########
service/src/main/java/org/apache/celeborn/server/common/service/store/db/mapper/ClusterInfoMapper.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * 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.server.common.service.store.db.mapper;
+
+import org.apache.ibatis.annotations.Insert;
+import org.apache.ibatis.annotations.Select;
+
+import org.apache.celeborn.server.common.service.model.ClusterInfo;
+
+public interface ClusterInfoMapper {
+
+ @Insert(
+ "INSERT INTO celeborn_cluster_info(name, namespace, endpoint, gmt_create, gmt_modify) "
+ + "values (#{name}, #{namespace}, #{endpoint}, #{gmtCreate}, #{gmtModify})")
Review Comment:
UPCASE all SQL keywords
```suggestion
+ "VALUES (#{name}, #{namespace}, #{endpoint}, #{gmtCreate}, #{gmtModify})")
```
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475787295
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4356,11 +4373,12 @@ object CelebornConf extends Logging {
val DYNAMIC_CONFIG_STORE_BACKEND: ConfigEntry[String] =
buildConf("celeborn.dynamicConfig.store.backend")
.categories("master", "worker")
- .doc("Store backend for dynamic config. Available options: NONE, FS. Note: NONE means disabling dynamic config store.")
+ .doc("Store backend for dynamic config service. Available options: NONE, FS, DB. Note: NONE means disabling " +
Review Comment:
Another concerns is only a few set of the configurations supports changed dynamiclly, where can user get such list?
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4356,11 +4373,12 @@ object CelebornConf extends Logging {
val DYNAMIC_CONFIG_STORE_BACKEND: ConfigEntry[String] =
buildConf("celeborn.dynamicConfig.store.backend")
.categories("master", "worker")
- .doc("Store backend for dynamic config. Available options: NONE, FS. Note: NONE means disabling dynamic config store.")
+ .doc("Store backend for dynamic config service. Available options: NONE, FS, DB. Note: NONE means disabling " +
Review Comment:
Another concern is only a few set of the configurations supports changed dynamiclly, where can user get such list?
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4356,11 +4373,12 @@ object CelebornConf extends Logging {
val DYNAMIC_CONFIG_STORE_BACKEND: ConfigEntry[String] =
buildConf("celeborn.dynamicConfig.store.backend")
.categories("master", "worker")
- .doc("Store backend for dynamic config. Available options: NONE, FS. Note: NONE means disabling dynamic config store.")
+ .doc("Store backend for dynamic config service. Available options: NONE, FS, DB. Note: NONE means disabling " +
Review Comment:
Another concern is only a few subset of the configurations supports changed dynamiclly, where can user get such list?
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475804152
##########
service/src/main/resources/db.sql:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.
+ */
+CREATE TABLE IF NOT EXISTS celeborn_cluster_info
+(
+ id int NOT NULL AUTO_INCREMENT,
+ name varchar(255) NOT NULL COMMENT 'celeborn cluster name',
+ namespace varchar(255) DEFAULT NULL COMMENT 'celeborn cluster namespace',
+ endpoint varchar(255) DEFAULT NULL COMMENT 'celeborn cluster endpoint',
+ gmt_create timestamp NULL DEFAULT NULL,
+ gmt_modify timestamp NULL DEFAULT NULL,
+ PRIMARY KEY (id),
+ UNIQUE KEY `index_cluster_unique_name` (`name`)
+);
+
+CREATE TABLE IF NOT EXISTS celeborn_cluster_system_config
+(
+ id int NOT NULL AUTO_INCREMENT,
+ cluster_id int NOT NULL,
+ config_key varchar(255) DEFAULT NULL,
+ config_value varchar(255) DEFAULT NULL,
+ type varchar(255) DEFAULT NULL COMMENT 'conf categories, such as quota',
Review Comment:
Can we enforce them with NOT NULL? Or they should be NULLABLE because what?
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477185167
##########
toolkit/db/mysql/celeborn-0.5.0-mysql.sql:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.
+ */
+CREATE TABLE IF NOT EXISTS celeborn_cluster_info
+(
+ id int NOT NULL AUTO_INCREMENT,
+ name varchar(255) NOT NULL COMMENT 'celeborn cluster name',
+ namespace varchar(255) DEFAULT NULL COMMENT 'celeborn cluster namespace',
+ endpoint varchar(255) DEFAULT NULL COMMENT 'celeborn cluster endpoint',
+ gmt_create timestamp NULL DEFAULT NULL,
+ gmt_modify timestamp NULL DEFAULT NULL,
+ PRIMARY KEY (id),
+ UNIQUE KEY `index_cluster_unique_name` (`name`)
+);
+
+CREATE TABLE IF NOT EXISTS celeborn_cluster_system_config
+(
+ id int NOT NULL AUTO_INCREMENT,
+ cluster_id int NOT NULL,
+ config_key varchar(255) DEFAULT NULL,
+ config_value varchar(255) DEFAULT NULL,
Review Comment:
should be NOT NULL too?
--
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-1054] Support db based dynamic config service [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong closed pull request #2273: [CELEBORN-1054] Support db based dynamic config service
URL: https://github.com/apache/incubator-celeborn/pull/2273
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477222387
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -367,7 +367,22 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
}
def dynamicConfigStoreBackend: String = get(DYNAMIC_CONFIG_STORE_BACKEND)
- def dynamicConfigRefreshInterval: Long = get(DYNAMIC_CONFIG_REFRESH_INTERVAL)
+ def dynamicConfigRefreshInterval: Long = get(DYNAMIC_CONFIG_REFRESH_INTERVAL).getOrElse(-1L)
Review Comment:
Emm... alright
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477306828
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4361,20 +4385,93 @@ object CelebornConf extends Logging {
val DYNAMIC_CONFIG_STORE_BACKEND: ConfigEntry[String] =
buildConf("celeborn.dynamicConfig.store.backend")
.categories("master", "worker")
- .doc("Store backend for dynamic config. Available options: NONE, FS. Note: NONE means disabling dynamic config store.")
+ .doc("Store backend for dynamic config service. Available options: FS, DB.")
.version("0.4.0")
.stringConf
.transform(_.toUpperCase(Locale.ROOT))
- .checkValues(Set("NONE", "FS"))
- .createWithDefault("NONE")
+ .checkValues(Set("FS", "DB"))
+ .createWithDefault("FS")
- val DYNAMIC_CONFIG_REFRESH_INTERVAL: ConfigEntry[Long] =
+ val DYNAMIC_CONFIG_REFRESH_INTERVAL: OptionalConfigEntry[Long] =
buildConf("celeborn.dynamicConfig.refresh.interval")
Review Comment:
Does the dynamic config work well when `celeborn.dynamicConfig.refresh.interval` is not configured?
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477203256
##########
NOTICE-binary:
##########
@@ -123,3 +123,15 @@ This software includes projects with other licenses -- see `doc/LICENSE.md`.
This product includes software developed by Google
Snappy: http://code.google.com/p/snappy/ (New BSD License)
+
+MyBatis
+Copyright 2010-2023
+
+This product includes software developed by
+The MyBatis Team (https://www.mybatis.org/).
+
+iBATIS
+ This product includes software developed by
+ The Apache Software Foundation (https://www.apache.org/).
+
+ Copyright 2010 The Apache Software Foundation
Review Comment:
the reminding content is required too, because those part is included in mybatis jar and shipped by our binary tgz
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477306828
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4361,20 +4385,93 @@ object CelebornConf extends Logging {
val DYNAMIC_CONFIG_STORE_BACKEND: ConfigEntry[String] =
buildConf("celeborn.dynamicConfig.store.backend")
.categories("master", "worker")
- .doc("Store backend for dynamic config. Available options: NONE, FS. Note: NONE means disabling dynamic config store.")
+ .doc("Store backend for dynamic config service. Available options: FS, DB.")
.version("0.4.0")
.stringConf
.transform(_.toUpperCase(Locale.ROOT))
- .checkValues(Set("NONE", "FS"))
- .createWithDefault("NONE")
+ .checkValues(Set("FS", "DB"))
+ .createWithDefault("FS")
- val DYNAMIC_CONFIG_REFRESH_INTERVAL: ConfigEntry[Long] =
+ val DYNAMIC_CONFIG_REFRESH_INTERVAL: OptionalConfigEntry[Long] =
buildConf("celeborn.dynamicConfig.refresh.interval")
Review Comment:
How does the user know that the dynamic config does not update well when `celeborn.dynamicConfig.refresh.interval` is not configured? IMO, the `celeborn.dynamicConfig.refresh.interval` is required.
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475689519
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -2201,6 +2210,14 @@ object CelebornConf extends Logging {
.timeConf(TimeUnit.MILLISECONDS)
.createWithDefaultString("30s")
+ val CLUSTER_NAME: ConfigEntry[String] =
+ buildConf("celeborn.cluster.name")
+ .categories("master", "worker")
+ .version("0.5.0")
+ .doc("celeborn cluster name")
Review Comment:
```suggestion
.doc("The name of celeborn cluster.")
```
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475787295
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4356,11 +4373,12 @@ object CelebornConf extends Logging {
val DYNAMIC_CONFIG_STORE_BACKEND: ConfigEntry[String] =
buildConf("celeborn.dynamicConfig.store.backend")
.categories("master", "worker")
- .doc("Store backend for dynamic config. Available options: NONE, FS. Note: NONE means disabling dynamic config store.")
+ .doc("Store backend for dynamic config service. Available options: NONE, FS, DB. Note: NONE means disabling " +
Review Comment:
Another concern is only a few subset of the configurations supports changing dynamiclly, where can user get such list?
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475785564
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4356,11 +4373,12 @@ object CelebornConf extends Logging {
val DYNAMIC_CONFIG_STORE_BACKEND: ConfigEntry[String] =
buildConf("celeborn.dynamicConfig.store.backend")
.categories("master", "worker")
- .doc("Store backend for dynamic config. Available options: NONE, FS. Note: NONE means disabling dynamic config store.")
+ .doc("Store backend for dynamic config service. Available options: NONE, FS, DB. Note: NONE means disabling " +
Review Comment:
IMO the current configurations may confusing users.
Both `NONE` and `FS` indicates reading conf from file, NONE does not support refresh but FS does. In such cases, would it be more clear to replace it with the following configurations:
- celeborn.configuration.provider: <FILE|DB>
- celeborn.configuration.refresh.interval: <None means disabled|Some(int) means enabled>
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477222205
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -367,7 +367,22 @@ class CelebornConf(loadDefaults: Boolean) extends Cloneable with Logging with Se
}
def dynamicConfigStoreBackend: String = get(DYNAMIC_CONFIG_STORE_BACKEND)
- def dynamicConfigRefreshInterval: Long = get(DYNAMIC_CONFIG_REFRESH_INTERVAL)
+ def dynamicConfigRefreshInterval: Long = get(DYNAMIC_CONFIG_REFRESH_INTERVAL).getOrElse(-1L)
Review Comment:
> why not just return a `Option[Long]`?
For java code we will get Option[Object] which may cause little boring.
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477161115
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4356,11 +4373,12 @@ object CelebornConf extends Logging {
val DYNAMIC_CONFIG_STORE_BACKEND: ConfigEntry[String] =
buildConf("celeborn.dynamicConfig.store.backend")
.categories("master", "worker")
- .doc("Store backend for dynamic config. Available options: NONE, FS. Note: NONE means disabling dynamic config store.")
+ .doc("Store backend for dynamic config service. Available options: NONE, FS, DB. Note: NONE means disabling " +
Review Comment:
> Another concern is only a few subset of the configurations supports changing dynamiclly, where can user get such list?
Later I will add a isDynamic properties to celebornConf, You can refer https://issues.apache.org/jira/browse/CELEBORN-1051
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477174122
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4356,11 +4373,12 @@ object CelebornConf extends Logging {
val DYNAMIC_CONFIG_STORE_BACKEND: ConfigEntry[String] =
buildConf("celeborn.dynamicConfig.store.backend")
.categories("master", "worker")
- .doc("Store backend for dynamic config. Available options: NONE, FS. Note: NONE means disabling dynamic config store.")
+ .doc("Store backend for dynamic config service. Available options: NONE, FS, DB. Note: NONE means disabling " +
Review Comment:
> IMO the current configurations may confusing users.
>
> Both `NONE` and `FS` indicates reading conf from file, NONE does not support refresh but FS does. In such cases, would it be more clear to replace it with the following configurations:
>
> * celeborn.configuration.provider: <FILE|DB>
> * celeborn.configuration.refresh.interval: <None means disabled|Some(int) means enabled>
Thank you @pan3793. The None Option already removed.
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4376,6 +4395,78 @@ object CelebornConf extends Logging {
.timeConf(TimeUnit.MILLISECONDS)
.createWithDefaultString("120s")
+ val DYNAMIC_CONFIG_STORE_DB_DRIVER_CLASS_NAME: ConfigEntry[String] =
+ buildConf("celeborn.dynamicConfig.store.db.driverClassName")
+ .categories("master", "worker")
+ .version("0.5.0")
+ .doc("The jdbc driver class name of db store backend.")
+ .stringConf
+ .createWithDefaultString("")
+
+ val DYNAMIC_CONFIG_STORE_DB_JDBC_URL: ConfigEntry[String] =
+ buildConf("celeborn.dynamicConfig.store.db.jdbcUrl")
+ .categories("master", "worker")
+ .version("0.5.0")
+ .doc("The jdbc url of db store backend.")
+ .stringConf
+ .createWithDefaultString("")
+
+ val DYNAMIC_CONFIG_STORE_DB_USERNAME: ConfigEntry[String] =
+ buildConf("celeborn.dynamicConfig.store.db.username")
+ .categories("master", "worker")
+ .version("0.5.0")
+ .doc("The username of db store backend.")
+ .stringConf
+ .createWithDefaultString("")
+
+ val DYNAMIC_CONFIG_STORE_DB_PASSWORD: ConfigEntry[String] =
+ buildConf("celeborn.dynamicConfig.store.db.password")
+ .categories("master", "worker")
+ .version("0.5.0")
+ .doc("The password of db store backend.")
+ .stringConf
+ .createWithDefaultString("")
+
+ val DYNAMIC_CONFIG_STORE_DB_CONNECTION_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.dynamicConfig.store.db.connectionTimeout")
+ .categories("master", "worker")
+ .version("0.5.0")
+ .doc("The connection timeout that a client will wait for a connection from the pool for db store backend.")
+ .timeConf(TimeUnit.MILLISECONDS)
+ .createWithDefaultString("30s")
+
+ val DYNAMIC_CONFIG_STORE_DB_IDLE_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.dynamicConfig.store.db.idleTimeout")
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-1054] Support db based dynamic config service [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#issuecomment-1926257668
Thanks all, 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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477185496
##########
toolkit/db/mysql/celeborn-0.5.0-mysql.sql:
##########
@@ -0,0 +1,56 @@
+/*
Review Comment:
the SQL file still not is included in the final binary tgz, right?
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475579794
##########
service/src/main/java/org/apache/celeborn/server/common/service/store/IServiceManager.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * 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.server.common.service.store;
+
+import java.util.List;
+
+import org.apache.celeborn.server.common.service.config.SystemConfig;
+import org.apache.celeborn.server.common.service.config.TenantConfig;
+import org.apache.celeborn.server.common.service.model.ClusterInfo;
+
+public interface IServiceManager {
+
+ int createClusterIfNotExists(ClusterInfo clusterInfo);
Review Comment:
@RexXiong, IMO, the name of this interface could be `createCluster`, no matter whether is with `IfNotExists` or `IfExists` for expanding this interface if needed.
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475584912
##########
service/src/main/resources/mybatis-config.xml:
##########
@@ -0,0 +1,43 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<!--
+ ~ 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.
+ -->
+
+<!DOCTYPE configuration
+ PUBLIC "-//mybatis.org//DTD Config 3.0//EN"
+ "http://mybatis.org/dtd/mybatis-3-config.dtd">
+<configuration>
+ <settings>
+ <setting name="mapUnderscoreToCamelCase" value="true" />
+ </settings>
+ <environments default="development">
+ <environment id="development">
+ <transactionManager type="JDBC"/>
+ <dataSource type="org.apache.celeborn.server.common.service.store.db.HikariDataSourceFactory">
Review Comment:
> MyBatis seems to support Java-based configuration too?
Do you mean remove all environment configuration from mybatis config file?
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#issuecomment-1925581953
Thanks @pan3793, 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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475871951
##########
service/src/test/resources/db.sql:
##########
@@ -0,0 +1,119 @@
+/*
+ * 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.
+ */
+
+CREATE SCHEMA IF NOT EXISTS TEST;
+
+SET SCHEMA TEST;
+DROP TABLE IF exists celeborn_cluster_info;
+DROP TABLE IF exists celeborn_cluster_system_config;
+DROP TABLE IF exists celeborn_cluster_tenant_config;
+
+CREATE TABLE celeborn_cluster_info
Review Comment:
why write `CREATE TABLE` statements again? we should use the user-oriented DDL file
--
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-1045] Support db based dynamic config server [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1473767078
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4356,11 +4373,12 @@ object CelebornConf extends Logging {
val DYNAMIC_CONFIG_STORE_BACKEND: ConfigEntry[String] =
buildConf("celeborn.dynamicConfig.store.backend")
.categories("master", "worker")
- .doc("Store backend for dynamic config. Available options: NONE, FS. Note: NONE means disabling dynamic config store.")
+ .doc("Store backend for dynamic config service. Available options: NONE, FS, DB. Note: NONE means disabling " +
Review Comment:
Could we describe mort details for `FS` and `DB` to tell users which options should configure after the `celeborn.dynamicConfig.store.backend` is `FS` or `DB`?
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4356,11 +4373,12 @@ object CelebornConf extends Logging {
val DYNAMIC_CONFIG_STORE_BACKEND: ConfigEntry[String] =
buildConf("celeborn.dynamicConfig.store.backend")
.categories("master", "worker")
- .doc("Store backend for dynamic config. Available options: NONE, FS. Note: NONE means disabling dynamic config store.")
+ .doc("Store backend for dynamic config service. Available options: NONE, FS, DB. Note: NONE means disabling " +
Review Comment:
Could we describe mort details for `FS` and `DB` to tell users which options should configure after the `celeborn.dynamicConfig.store.backend` is `FS` or `DB`?
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475570462
##########
service/src/main/java/org/apache/celeborn/server/common/service/store/IServiceManager.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * 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.server.common.service.store;
+
+import java.util.List;
+
+import org.apache.celeborn.server.common.service.config.SystemConfig;
+import org.apache.celeborn.server.common.service.config.TenantConfig;
+import org.apache.celeborn.server.common.service.model.ClusterInfo;
+
+public interface IServiceManager {
+
+ int createClusterIfNotExists(ClusterInfo clusterInfo);
Review Comment:
>
Currently, we have no need to createCluster without IfNotExists, we can expand this method if this is a need.
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475792026
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4376,6 +4395,78 @@ object CelebornConf extends Logging {
.timeConf(TimeUnit.MILLISECONDS)
.createWithDefaultString("120s")
+ val DYNAMIC_CONFIG_STORE_DB_DRIVER_CLASS_NAME: ConfigEntry[String] =
+ buildConf("celeborn.dynamicConfig.store.db.driverClassName")
+ .categories("master", "worker")
+ .version("0.5.0")
+ .doc("The jdbc driver class name of db store backend.")
+ .stringConf
+ .createWithDefaultString("")
+
+ val DYNAMIC_CONFIG_STORE_DB_JDBC_URL: ConfigEntry[String] =
+ buildConf("celeborn.dynamicConfig.store.db.jdbcUrl")
+ .categories("master", "worker")
+ .version("0.5.0")
+ .doc("The jdbc url of db store backend.")
+ .stringConf
+ .createWithDefaultString("")
+
+ val DYNAMIC_CONFIG_STORE_DB_USERNAME: ConfigEntry[String] =
+ buildConf("celeborn.dynamicConfig.store.db.username")
+ .categories("master", "worker")
+ .version("0.5.0")
+ .doc("The username of db store backend.")
+ .stringConf
+ .createWithDefaultString("")
+
+ val DYNAMIC_CONFIG_STORE_DB_PASSWORD: ConfigEntry[String] =
+ buildConf("celeborn.dynamicConfig.store.db.password")
+ .categories("master", "worker")
+ .version("0.5.0")
+ .doc("The password of db store backend.")
+ .stringConf
+ .createWithDefaultString("")
+
+ val DYNAMIC_CONFIG_STORE_DB_CONNECTION_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.dynamicConfig.store.db.connectionTimeout")
+ .categories("master", "worker")
+ .version("0.5.0")
+ .doc("The connection timeout that a client will wait for a connection from the pool for db store backend.")
+ .timeConf(TimeUnit.MILLISECONDS)
+ .createWithDefaultString("30s")
+
+ val DYNAMIC_CONFIG_STORE_DB_IDLE_TIMEOUT: ConfigEntry[Long] =
+ buildConf("celeborn.dynamicConfig.store.db.idleTimeout")
Review Comment:
a leason we get from handling ratis configurations, it's better to use a prefix to passthrough all underlying(in this case, hikari) configurations.
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1475866859
##########
service/src/main/java/org/apache/celeborn/server/common/service/store/db/mapper/ClusterInfoMapper.java:
##########
@@ -0,0 +1,35 @@
+/*
+ * 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.server.common.service.store.db.mapper;
+
+import org.apache.ibatis.annotations.Insert;
+import org.apache.ibatis.annotations.Select;
+
+import org.apache.celeborn.server.common.service.model.ClusterInfo;
+
+public interface ClusterInfoMapper {
+
+ @Insert(
+ "INSERT INTO celeborn_cluster_info(name, namespace, endpoint, gmt_create, gmt_modify) "
+ + "values (#{name}, #{namespace}, #{endpoint}, #{gmtCreate}, #{gmtModify})")
+ void insert(ClusterInfo clusterInfo);
+
+ @Select(
+ "SELECT id, name, namespace, endpoint, gmt_create, gmt_modify from celeborn_cluster_info where name = #{clusterName}")
Review Comment:
```suggestion
"SELECT id, name, namespace, endpoint, gmt_create, gmt_modify FROM celeborn_cluster_info WHERE name = #{clusterName}")
```
--
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-1045] Support db based dynamic config server [incubator-celeborn]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#issuecomment-1917069590
## [Codecov](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `83 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`9392ed4`)](https://app.codecov.io/gh/apache/incubator-celeborn/commit/9392ed492c9f819abf7e9cefda7dff6ad644e585?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 48.20% compared to head [(`fd9a9f5`)](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 48.53%.
> Report is 1 commits behind head on main.
> :exclamation: Current head fd9a9f5 differs from pull request most recent head b1654c4. Consider uploading reports for the commit b1654c4 to get more accurate results
| [Files](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...rver/common/service/model/ClusterTenantConfig.java](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmljZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvY2VsZWJvcm4vc2VydmVyL2NvbW1vbi9zZXJ2aWNlL21vZGVsL0NsdXN0ZXJUZW5hbnRDb25maWcuamF2YQ==) | 53.34% | [21 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...mmon/service/store/db/HikariDataSourceFactory.java](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmljZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvY2VsZWJvcm4vc2VydmVyL2NvbW1vbi9zZXJ2aWNlL3N0b3JlL2RiL0hpa2FyaURhdGFTb3VyY2VGYWN0b3J5LmphdmE=) | 0.00% | [15 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../common/service/store/db/DbServiceManagerImpl.java](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmljZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvY2VsZWJvcm4vc2VydmVyL2NvbW1vbi9zZXJ2aWNlL3N0b3JlL2RiL0RiU2VydmljZU1hbmFnZXJJbXBsLmphdmE=) | 77.20% | [12 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...che/celeborn/server/common/service/utils/JSON.java](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmljZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvY2VsZWJvcm4vc2VydmVyL2NvbW1vbi9zZXJ2aWNlL3V0aWxzL0pTT04uamF2YQ==) | 0.00% | [11 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...rver/common/service/model/ClusterSystemConfig.java](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmljZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvY2VsZWJvcm4vc2VydmVyL2NvbW1vbi9zZXJ2aWNlL21vZGVsL0NsdXN0ZXJTeXN0ZW1Db25maWcuamF2YQ==) | 68.19% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...eborn/server/common/service/model/ClusterInfo.java](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmljZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvY2VsZWJvcm4vc2VydmVyL2NvbW1vbi9zZXJ2aWNlL21vZGVsL0NsdXN0ZXJJbmZvLmphdmE=) | 68.43% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...r/common/service/config/BaseConfigServiceImpl.java](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmljZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvY2VsZWJvcm4vc2VydmVyL2NvbW1vbi9zZXJ2aWNlL2NvbmZpZy9CYXNlQ29uZmlnU2VydmljZUltcGwuamF2YQ==) | 86.37% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...rver/common/service/store/db/DBSessionFactory.java](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmljZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvY2VsZWJvcm4vc2VydmVyL2NvbW1vbi9zZXJ2aWNlL3N0b3JlL2RiL0RCU2Vzc2lvbkZhY3RvcnkuamF2YQ==) | 72.73% | [1 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...on/service/config/DynamicConfigServiceFactory.java](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmljZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvY2VsZWJvcm4vc2VydmVyL2NvbW1vbi9zZXJ2aWNlL2NvbmZpZy9EeW5hbWljQ29uZmlnU2VydmljZUZhY3RvcnkuamF2YQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ver/common/service/config/DbConfigServiceImpl.java](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmljZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvY2VsZWJvcm4vc2VydmVyL2NvbW1vbi9zZXJ2aWNlL2NvbmZpZy9EYkNvbmZpZ1NlcnZpY2VJbXBsLmphdmE=) | 92.31% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| ... and [1 more](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #2273 +/- ##
==========================================
+ Coverage 48.20% 48.53% +0.33%
==========================================
Files 197 206 +9
Lines 12301 12537 +236
Branches 1073 1080 +7
==========================================
+ Hits 5929 6084 +155
- Misses 5983 6060 +77
- Partials 389 393 +4
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-celeborn/pull/2273?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-1045] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#issuecomment-1917066438
My suggestion may be a bit paranoid: can we avoid using the ORM framework and involve additional configuration files other than `celeborn-defaults.conf`? specific to this PR, I mean iBaits and Hikari
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477205889
##########
service/src/main/java/org/apache/celeborn/server/common/service/store/db/DBSessionFactory.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.server.common.service.store.db;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.Properties;
+
+import javax.sql.DataSource;
+
+import com.zaxxer.hikari.HikariConfig;
+import com.zaxxer.hikari.HikariDataSource;
+import org.apache.ibatis.mapping.Environment;
+import org.apache.ibatis.session.Configuration;
+import org.apache.ibatis.session.SqlSessionFactory;
+import org.apache.ibatis.session.SqlSessionFactoryBuilder;
+import org.apache.ibatis.transaction.TransactionFactory;
+import org.apache.ibatis.transaction.jdbc.JdbcTransactionFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.celeborn.common.CelebornConf;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterInfoMapper;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterSystemConfigMapper;
+import org.apache.celeborn.server.common.service.store.db.mapper.ClusterTenantConfigMapper;
+
+public class DBSessionFactory {
+ private static final Logger LOG = LoggerFactory.getLogger(DBSessionFactory.class);
+ private static volatile SqlSessionFactory _instance;
+
+ public static SqlSessionFactory get(CelebornConf celebornConf) throws IOException {
+ if (_instance == null) {
+ synchronized (DBSessionFactory.class) {
+ if (_instance == null) {
+ Properties properties = new Properties();
+ properties.setProperty(
+ "driverClassName", celebornConf.dynamicConfigStoreDbHikariDriverClassName());
+ properties.setProperty("jdbcUrl", celebornConf.dynamicConfigStoreDbHikariJdbcUrl());
+ properties.setProperty("username", celebornConf.dynamicConfigStoreDbHikariUsername());
+ properties.setProperty("password", celebornConf.dynamicConfigStoreDbHikariPassword());
+ properties.setProperty(
+ "connectionTimeout",
+ String.valueOf(celebornConf.dynamicConfigStoreDbHikariConnectionTimeout()));
+ properties.setProperty(
+ "idleTimeout", String.valueOf(celebornConf.dynamicConfigStoreDbHikariIdleTimeout()));
+ properties.setProperty(
+ "maxLifetime", String.valueOf(celebornConf.dynamicConfigStoreDbHikariMaxLifetime()));
+ properties.setProperty(
+ "maximumPoolSize",
+ String.valueOf(celebornConf.dynamicConfigStoreDbHikariMaximumPoolSize()));
+
+ for (Map.Entry<String, String> dbPropertiesEntry :
+ celebornConf.dynamicConfigStoreDbHikariCustomConfigs().entrySet()) {
+ properties.setProperty(
+ dbPropertiesEntry.getKey().replace("celeborn.dynamicConfig.store.db.hikari.", ""),
+ dbPropertiesEntry.getValue());
+ }
+
+ HikariConfig config = new HikariConfig(properties);
+ DataSource dataSource = new HikariDataSource(config);
+
+ TransactionFactory transactionFactory = new JdbcTransactionFactory();
+ Environment environment = new Environment("celeborn", transactionFactory, dataSource);
+
+ Configuration configuration = new Configuration(environment);
+ configuration.setMapUnderscoreToCamelCase(true);
Review Comment:
is it required?
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477210111
##########
service/src/test/resources/celeborn-0.5.0-h2.sql:
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.
+ */
+
+CREATE SCHEMA IF NOT EXISTS TEST;
+
+SET SCHEMA TEST;
+DROP TABLE IF exists celeborn_cluster_info;
+DROP TABLE IF exists celeborn_cluster_system_config;
+DROP TABLE IF exists celeborn_cluster_tenant_config;
+
+CREATE TABLE celeborn_cluster_info
+(
+ id int NOT NULL AUTO_INCREMENT,
+ name varchar(255) NOT NULL COMMENT 'celeborn cluster name',
+ namespace varchar(255) DEFAULT NULL COMMENT 'celeborn cluster namespace',
+ endpoint varchar(255) DEFAULT NULL COMMENT 'celeborn cluster endpoint',
+ gmt_create timestamp NULL DEFAULT NULL,
+ gmt_modify timestamp NULL DEFAULT NULL,
+ PRIMARY KEY (id),
+ UNIQUE KEY `index_cluster_unique_name` (`name`)
+ );
+
+CREATE TABLE celeborn_cluster_system_config
+(
+ id int NOT NULL AUTO_INCREMENT,
+ cluster_id int NOT NULL,
+ config_key varchar(255) DEFAULT NULL,
+ config_value varchar(255) DEFAULT NULL,
+ type varchar(255) DEFAULT NULL COMMENT 'conf categories, such as quota',
+ gmt_create timestamp NOT NULL,
+ gmt_modify timestamp NOT NULL,
+ PRIMARY KEY (id),
+ UNIQUE KEY `index_unique_system_config_key` (`cluster_id`, `config_key`)
+ );
+
+CREATE TABLE celeborn_cluster_tenant_config
+(
+ id int NOT NULL AUTO_INCREMENT,
+ cluster_id int NOT NULL,
+ tenant_id varchar(255) NOT NULL,
+ level varchar(255) NOT NULL COMMENT 'config level, valid level is TENANT,USER',
+ `user` varchar(255) DEFAULT NULL COMMENT 'tenant sub user',
+ config_key varchar(255) NOT NULL,
+ config_value varchar(255) NOT NULL,
+ type varchar(255) DEFAULT NULL COMMENT 'conf categories, such as quota',
+ gmt_create timestamp NOT NULL,
+ gmt_modify timestamp NOT NULL,
+ PRIMARY KEY (id),
+ UNIQUE KEY `index_unique_tenant_config_key` (`cluster_id`, `tenant_id`, `user`, `config_key`)
+ );
+
+
+INSERT INTO celeborn_cluster_info (`id`, `name`, `namespace`, `endpoint`, `gmt_create`, `gmt_modify`)
Review Comment:
ideally, we should split this file into two, and the schema one should be exposed to the end-user as same as MySQL, but keep the data file for UT only.
--
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-1054] Support db based dynamic config server [incubator-celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #2273:
URL: https://github.com/apache/incubator-celeborn/pull/2273#discussion_r1477625344
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4361,12 +4386,20 @@ object CelebornConf extends Logging {
val DYNAMIC_CONFIG_STORE_BACKEND: ConfigEntry[String] =
buildConf("celeborn.dynamicConfig.store.backend")
.categories("master", "worker")
- .doc("Store backend for dynamic config. Available options: NONE, FS. Note: NONE means disabling dynamic config store.")
+ .doc("Store backend for dynamic config service. Available options: FS, DB.")
.version("0.4.0")
.stringConf
.transform(_.toUpperCase(Locale.ROOT))
- .checkValues(Set("NONE", "FS"))
- .createWithDefault("NONE")
+ .checkValues(Set("FS", "DB"))
+ .createWithDefault("FS")
+
+ val DYNAMIC_CONFIG_REFRESH_ENABLED: ConfigEntry[Boolean] =
+ buildConf("celeborn.dynamicConfig.refresh.enabled")
Review Comment:
```suggestion
buildConf("celeborn.dynamicConfig.enabled")
```
##########
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala:
##########
@@ -4361,12 +4386,20 @@ object CelebornConf extends Logging {
val DYNAMIC_CONFIG_STORE_BACKEND: ConfigEntry[String] =
buildConf("celeborn.dynamicConfig.store.backend")
.categories("master", "worker")
- .doc("Store backend for dynamic config. Available options: NONE, FS. Note: NONE means disabling dynamic config store.")
+ .doc("Store backend for dynamic config service. Available options: FS, DB.")
.version("0.4.0")
.stringConf
.transform(_.toUpperCase(Locale.ROOT))
- .checkValues(Set("NONE", "FS"))
- .createWithDefault("NONE")
+ .checkValues(Set("FS", "DB"))
+ .createWithDefault("FS")
+
+ val DYNAMIC_CONFIG_REFRESH_ENABLED: ConfigEntry[Boolean] =
+ buildConf("celeborn.dynamicConfig.refresh.enabled")
+ .categories("master", "worker")
+ .version("0.5.0")
+ .doc("Whether to enable configuration refresher.")
Review Comment:
```suggestion
.doc("Whether to enable dynamic configuration.")
```
--
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