[CELEBORN-1052] Introduce dynamic ConfigService at SystemLevel and TenantLevel #2100
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2100 +/- ##
==========================================
+ Coverage 46.76% 46.95% +0.20%
==========================================
Files 166 173 +7
Lines 10695 10929 +234
Branches 977 1009 +32
==========================================
+ Hits 5000 5131 +131
- Misses 5373 5466 +93
- Partials 322 332 +10 ☔ View full report in Codecov by Sentry. |
SteNicholas
reviewed
Nov 15, 2023
There was a problem hiding this comment.
@RexXiong, I left some comments for the dynamic config implementation.
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala
Outdated
Show resolved
Hide resolved
common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala
Outdated
Show resolved
Hide resolved
service/src/main/scala/org/apache/celeborn/server/common/service/config/ConfigService.java
Show resolved
Hide resolved
service/src/main/scala/org/apache/celeborn/server/common/service/config/DynamicConfig.java
Show resolved
Hide resolved
...main/scala/org/apache/celeborn/server/common/service/config/DynamicConfigServiceFactory.java
Show resolved
Hide resolved
...main/scala/org/apache/celeborn/server/common/service/config/DynamicConfigServiceFactory.java
Show resolved
Hide resolved
service/src/main/scala/org/apache/celeborn/server/common/service/config/ConfigService.java
Show resolved
Hide resolved
...ice/src/main/scala/org/apache/celeborn/server/common/service/config/FsConfigServiceImpl.java
Show resolved
Hide resolved
...ice/src/test/scala/org/apache/celeborn/server/common/service/config/ConfigServiceSuiteJ.java
Show resolved
Hide resolved
RexXiong
force-pushed
the
CELEBORN-1052
branch
from
0b652a0 to
dfb3150
Compare
SteNicholas
reviewed
Nov 17, 2023
...ice/src/main/scala/org/apache/celeborn/server/common/service/config/FsConfigServiceImpl.java
Outdated
Show resolved
Hide resolved
FMX
approved these changes
Nov 24, 2023
RexXiong
pushed a commit
that referenced
this pull request
Dec 4, 2023
…vel and TenantLevel ### What changes were proposed in this pull request? Follow up #2100. Mainly changes the package from scala to java of the codes in #2100. Meanwhile, `FsConfigServiceImpl#refresh` should directly return instead of refreshing configs. ### Why are the changes needed? This PR follow up dynamic `ConfigService` at `SystemLevel` and `TenantLevel`, Dynamic configuration is a type of configuration that can be changed at runtime as needed in #2100. The implementation of `ConfigService` is based on Java codes, which are put into Scala package and cause that the spotless plugin does not format well. After the changes of the pull request, there are much code style changes generated from the package moving behavior. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? `ConfigServiceSuiteJ`. Closes #2125 from SteNicholas/CELEBORN-1052. Authored-by: SteNicholas <[email protected]> Signed-off-by: Shuang <[email protected]>
SteNicholas
added a commit
that referenced
this pull request
Feb 28, 2024
… and config service ### What changes were proposed in this pull request? Introduce `configuration.md` to document dynamic config and config service. ### Why are the changes needed? `DynamicConfig` and `ConfigService` have already been supported in #2100, which should be documented to introduce the feature. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? No. Closes #2336 from SteNicholas/CELEBORN-1286. Authored-by: SteNicholas <[email protected]> Signed-off-by: SteNicholas <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This PR introduce dynamic ConfigService at SystemLevel and TenantLevel, Dynamic configuration is a type of configuration that can be changed at runtime as needed. It can be used at system level/tenant level. When applying dynamic configuration, the priority order is as follows: tenant level overrides system level, which in turn overrides static configuration(CelebornConf). This means that if a configuration is defined at the tenant level, it will be used instead of the system level or static configuration(CelebornConf). If the tenant-level configuration is missing,
the system-level configuration will be used. If the system-level configuration is also missing, CelebornConf
will be used as the default value.
There are several other tasks related to this feature that will be implemented in the future.
Why are the changes needed?
The current configuration of the server (CelebornConf) is static. When the configuration is changed, the service needs to be restarted. This PR introduces a dynamic configuration solution. The server side can use dynamic configuration as needed. At the same time, it is considered that the tenant level will be supported in the future (such as supporting tenant level dynamic quota control) configuration, so this time we will also consider supporting dynamic tenant-level configuration, and this PR will provide a default implementation based on the file system.
Does this PR introduce any user-facing change?
No
How was this patch tested?
UT