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

[fix-11404]: make the common.properties to configurable on values.yaml #11441

Merged
merged 1 commit into from
Aug 19, 2022
Merged

[fix-11404]: make the common.properties to configurable on values.yaml #11441

merged 1 commit into from
Aug 19, 2022

Conversation

itzhang89
Copy link
Contributor

@itzhang89 itzhang89 commented Aug 12, 2022

and set the resource.storage.type default to hdfs

Purpose of the pull request

fix #11404(#11404)

Brief change log

make the common.properties to configurable on values.yaml and set the default storage type to hdfs and fs.defaultFS

conf:
  common:
    resource.storage.type: HDFS
    fs.defaultFS: file:///

Verify this pull request

This pull request is code cleanup without any test coverage.

@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2022

Codecov Report

Merging #11441 (419f9a9) into dev (abd5798) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##                dev   #11441      +/-   ##
============================================
- Coverage     39.56%   39.55%   -0.01%     
  Complexity     4630     4630              
============================================
  Files           980      980              
  Lines         37282    37278       -4     
  Branches       4180     4179       -1     
============================================
- Hits          14749    14746       -3     
+ Misses        20987    20986       -1     
  Partials       1546     1546              
Impacted Files Coverage Δ
...erver/master/processor/queue/TaskEventService.java 75.00% <0.00%> (-5.36%) ⬇️
...cheduler/server/master/utils/DependentExecute.java 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@aspexdaniel
Copy link

@yutianaiqingtian maybe we should replace the values in common.properties from common.configmap.* in values.yaml?
since we already have these configs like RESOURCE_STORAGE_TYPE, FS_DEFAULT_FS, FS_S3A_ENDPOINT,
putting everything in values.yaml is a bit redundant.

@itzhang89
Copy link
Contributor Author

itzhang89 commented Aug 12, 2022

@aspexdaniel exactly It's a bit redundant. but the value the common.configmap.RESOURCE_STORAGE_TYPE is only check on the helm chart and cannot take effect by the application. I think maybe we should split the common.configmap to two part. one is for the environment and another is for the common.properties.

@itzhang89 itzhang89 closed this Aug 12, 2022
@itzhang89 itzhang89 reopened this Aug 12, 2022
 and set the resource.storage.type default to hdfs
@aspexdaniel
Copy link

@yutianaiqingtian this is strange though, because I tested 2.0.6 chart with common.configmap.* points to Minio , it worked. From the code I feel it's not that big different between the two version for k8s helm chart.

@sonarcloud
Copy link

sonarcloud bot commented Aug 17, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 16 Code Smells

20.0% 20.0% Coverage
0.7% 0.7% Duplication

Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

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

+1

@SbloodyS SbloodyS added this to the 3.0.1 milestone Aug 19, 2022
@SbloodyS SbloodyS merged commit 1c15a7d into apache:dev Aug 19, 2022
@SbloodyS
Copy link
Member

Thanks for your contribution. This is a good beginning. Considering that it's your first contribution, I think we can get deep communication, you can contact me through mail or add WeChat(SbloodyS), when mail or added, please tell me who you are. I think I can help to familiar with the Dolphinscheduler if you meet with problems. @yutianaiqingtian

zhuxt2015 pushed a commit to zhuxt2015/dolphinscheduler that referenced this pull request Aug 21, 2022
@itzhang89 itzhang89 deleted the fix-11404 branch August 23, 2022 03:42
zhuangchong pushed a commit to zhuangchong/incubator-dolphinscheduler that referenced this pull request Sep 15, 2022
@zhuangchong zhuangchong added the release cherry-pick Mark this issue/PR had cherry-pick for release version label Sep 15, 2022
caishunfeng pushed a commit that referenced this pull request Sep 16, 2022
#11441) (#11967)

and set the resource.storage.type default to hdfs

Co-authored-by: 爱穿格子裤 <30542109+yutianaiqingtian@users.noreply.github.com>
xdu-chenrj pushed a commit to xdu-chenrj/dolphinscheduler that referenced this pull request Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend first time contributor First-time contributor release cherry-pick Mark this issue/PR had cherry-pick for release version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] [UI] Storage not enabled
6 participants