Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[CELEBORN-1054] Support db based dynamic config service #2273

Closed
wants to merge 24 commits into from

Conversation

RexXiong
Copy link
Contributor

@RexXiong RexXiong commented Jan 30, 2024

What changes were proposed in this pull request?

Support database based store backend implementation for dynamic configuration management

Why are the changes needed?

Currently celeborn provides FsConfigServiceImpl implementation for dynamic config service which is based on file system, We cloud Support database based store backend implementation.

Does this PR introduce any user-facing change?

No

How was this patch tested?

  • ConfigServiceSuiteJ#testDbConfig

@pan3793
Copy link
Member

pan3793 commented Jan 30, 2024

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 iBatis and Hikari

With a thin wrapper, we can write the vanilla JDBC API like

   implicit val hikariDataSource = new HikariDataSource(...)

   // query case
   JdbcUtils.executeQueryWithRowMapper(
      s"""SELECT item1, item2
         |FROM table
         |WHERE state=?
         |ORDER BY create_time ASC LIMIT 1
         |""".stripMargin) { stmt =>
      // set parameter
      stmt.setString(1, "balabala")
    } { resultSet =>
      // map the ResultSet to data struct, usually a POJO
      resultSet.getString(1)
    }

Copy link

codecov bot commented Jan 30, 2024

Codecov Report

Attention: 74 lines in your changes are missing coverage. Please review.

Comparison is base (a6682d5) 47.82% compared to head (6519de1) 48.55%.
Report is 5 commits behind head on main.

Files Patch % Lines
...rver/common/service/model/ClusterTenantConfig.java 48.89% 23 Missing ⚠️
.../common/service/store/db/DbServiceManagerImpl.java 77.20% 12 Missing and 1 partial ⚠️
...r/common/service/config/BaseConfigServiceImpl.java 59.26% 10 Missing and 1 partial ⚠️
...eleborn/server/common/service/utils/JsonUtils.java 0.00% 11 Missing ⚠️
...rver/common/service/model/ClusterSystemConfig.java 77.28% 5 Missing ⚠️
...eborn/server/common/service/model/ClusterInfo.java 78.95% 4 Missing ⚠️
...on/service/config/DynamicConfigServiceFactory.java 0.00% 3 Missing ⚠️
...rver/common/service/store/db/DBSessionFactory.java 94.74% 1 Missing and 1 partial ⚠️
...ver/common/service/config/DbConfigServiceImpl.java 92.86% 0 Missing and 1 partial ⚠️
...orn/server/common/service/config/TenantConfig.java 90.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2273      +/-   ##
==========================================
+ Coverage   47.82%   48.55%   +0.73%     
==========================================
  Files         200      208       +8     
  Lines       12449    12762     +313     
  Branches     1088     1096       +8     
==========================================
+ Hits         5953     6195     +242     
- Misses       6103     6171      +68     
- Partials      393      396       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@RexXiong
Copy link
Contributor Author

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 iBatis and Hikari

With a thin wrapper, we can write the vanilla JDBC API like

   implicit val hikariDataSource = new HikariDataSource(...)

   // query case
   JdbcUtils.executeQueryWithRowMapper(
      s"""SELECT item1, item2
         |FROM table
         |WHERE state=?
         |ORDER BY create_time ASC LIMIT 1
         |""".stripMargin) { stmt =>
      // set parameter
      stmt.setString(1, "balabala")
    } { resultSet =>
      // map the ResultSet to data struct, usually a POJO
      resultSet.getString(1)
    }

After offline discussion, we currently tend to keep using the orm (ibatis) framework, and at the same time unify Database-related configurations to CelebornConf.

@RexXiong RexXiong marked this pull request as ready for review January 31, 2024 06:01
@RexXiong RexXiong changed the title [CELEBORN-1045] Support db based dynamic config server [CELEBORN-1054] Support db based dynamic config server Feb 2, 2024
Copy link
Member

@pan3793 pan3793 left a comment

Choose a reason for hiding this comment

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

Some minor issues, almost good

Copy link
Member

@SteNicholas SteNicholas left a comment

Choose a reason for hiding this comment

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

LGTM.

@RexXiong RexXiong changed the title [CELEBORN-1054] Support db based dynamic config server [CELEBORN-1054] Support db based dynamic config service Feb 5, 2024
@RexXiong RexXiong closed this in d89dcf0 Feb 5, 2024
@RexXiong
Copy link
Contributor Author

RexXiong commented Feb 5, 2024

Thanks all, merge to main(v0.5.0)

tiny-dust pushed a commit to tiny-dust/incubator-celeborn that referenced this pull request Feb 20, 2024
### What changes were proposed in this pull request?

Support database based store backend implementation for dynamic configuration management

### Why are the changes needed?

Currently celeborn provides `FsConfigServiceImpl` implementation for dynamic config service which is based on file system, We cloud Support database based store backend implementation.

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

- `ConfigServiceSuiteJ#testDbConfig`

Closes apache#2273 from RexXiong/CELEBORN-1054.

Authored-by: Shuang <lvshuang.xjs@alibaba-inc.com>
Signed-off-by: Shuang <lvshuang.xjs@alibaba-inc.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants