Skip to content

[#1110] improvement(coordinator): better support of remote storage config - #1329

Merged
zuston merged 12 commits into
apache:masterfrom
zuston:issue-1110
Nov 30, 2023
Merged

[#1110] improvement(coordinator): better support of remote storage config#1329
zuston merged 12 commits into
apache:masterfrom
zuston:issue-1110

Conversation

@zuston

@zuston zuston commented Nov 24, 2023

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Introduce the new DynamicClientConfigService to provide the pluggable reader for config file multi format.
And it also is compatible with the original dynamic config format.

This PR introduces the new yaml parser, which is recommended in the future, the storage's hadoop conf could be xml style or k1:v1 . The example as follows.

rssClientConf:
  k1: v1
  k2: v2

remoteStorageInfos:
  hdfs://a-ns01: |+
    <configuration>
        <property>
          <name>k1</name>
          <value>v1</value>
        </property>

        <property>
          <name>k2</name>
          <value>v2</value>
        </property>
    </configuration>

  hdfs://x-ns01:
    k1: v1
    k2: v2

Why are the changes needed?

Fix: #1110

Does this PR introduce any user-facing change?

No.

How was this patch tested?

  1. UTs

@zuston
zuston requested review from roryqi and zhengchenyu November 24, 2023 06:10
@zuston

zuston commented Nov 24, 2023

Copy link
Copy Markdown
Member Author

Could you help review this ? @zhengchenyu

@roryqi
roryqi requested a review from LuciferYang November 24, 2023 06:17
@roryqi

roryqi commented Nov 24, 2023

Copy link
Copy Markdown
Contributor

@LuciferYang Could you help me review this pr?

@codecov-commenter

codecov-commenter commented Nov 24, 2023

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 85.78947% with 27 lines in your changes missing coverage. Please review.

Project coverage is 54.19%. Comparing base (51e02d0) to head (d1f41b1).
Report is 336 commits behind head on master.

Files Patch % Lines
...fle/coordinator/conf/DynamicClientConfService.java 78.57% 12 Missing and 3 partials ⚠️
...apache/uniffle/coordinator/ApplicationManager.java 91.17% 2 Missing and 1 partial ⚠️
...he/uniffle/coordinator/CoordinatorGrpcService.java 0.00% 3 Missing ⚠️
.../apache/uniffle/coordinator/CoordinatorServer.java 50.00% 1 Missing and 1 partial ⚠️
...iffle/coordinator/conf/LegacyClientConfParser.java 94.73% 2 Missing ⚠️
...uniffle/coordinator/conf/YamlClientConfParser.java 92.30% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1329      +/-   ##
============================================
+ Coverage     53.47%   54.19%   +0.71%     
- Complexity     2695     2707      +12     
============================================
  Files           411      398      -13     
  Lines         23644    21505    -2139     
  Branches       2006     2036      +30     
============================================
- Hits          12643    11654     -989     
+ Misses        10225     9138    -1087     
+ Partials        776      713      -63     

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

@zuston
zuston requested a review from LuciferYang November 27, 2023 02:28
@zuston

zuston commented Nov 28, 2023

Copy link
Copy Markdown
Member Author

Could you help review again? @LuciferYang

@zuston

zuston commented Nov 29, 2023

Copy link
Copy Markdown
Member Author

Gentle ping @LuciferYang

LuciferYang
LuciferYang previously approved these changes Nov 29, 2023

@LuciferYang LuciferYang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1, LGTM apart from the comments below

Comment thread coordinator/src/main/java/org/apache/uniffle/coordinator/ApplicationManager.java Outdated
Comment thread coordinator/src/test/resources/dynamicClientConf.legacy Outdated
@zuston
zuston merged commit 64dd847 into apache:master Nov 30, 2023
@zuston

zuston commented Nov 30, 2023

Copy link
Copy Markdown
Member Author

Thanks @LuciferYang . Merged.

@zuston
zuston deleted the issue-1110 branch November 30, 2023 04:14
zuston added a commit to zuston/incubator-uniffle that referenced this pull request Jan 18, 2024
…orage config format (apache#1329)

Introduce the new `DynamicClientConfigService` to provide the pluggable reader for config file multi format.
And it also is compatible with the original dynamic config format.

This PR introduces the new yaml parser, which is recommended in the future, the storage's hadoop conf could be xml style or k1:v1 . The example as follows.

```yaml
rssClientConf:
  k1: v1
  k2: v2

remoteStorageInfos:
  hdfs://a-ns01: |+
    <configuration>
        <property>
          <name>k1</name>
          <value>v1</value>
        </property>

        <property>
          <name>k2</name>
          <value>v2</value>
        </property>
    </configuration>

  hdfs://x-ns01:
    k1: v1
    k2: v2
```

Fix: apache#1110

No.

1. UTs
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.

[Improvement] the value of remote storage conf should support comma.

4 participants