You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "zuston (via GitHub)" <gi...@apache.org> on 2023/12/13 08:43:05 UTC
[PR] [#1364] feat(client): introduce option to control whether to use local hadoop conf for remote storage by default [incubator-uniffle]
zuston opened a new pull request, #1370:
URL: https://github.com/apache/incubator-uniffle/pull/1370
### What changes were proposed in this pull request?
Introduce the config to control whether to use local hadoop conf for remote storage by default
### Why are the changes needed?
I want to deploy one uniffle cluster to serve for spark jobs in multiple hadoop clusters. So I hope the remote storage config could be loaded from the default hdfs-site.xml in the classpath.
### Does this PR introduce _any_ user-facing change?
Yes.
### How was this patch tested?
Internal tests.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1364] feat(client): introduce option to control whether to use local hadoop conf for remote storage by default [incubator-uniffle]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1370:
URL: https://github.com/apache/incubator-uniffle/pull/1370#issuecomment-1853503568
## [Codecov](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1370?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `9 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`b2fe152`)](https://app.codecov.io/gh/apache/incubator-uniffle/commit/b2fe152869f3308b1b112f258ede7bdbf94b3907?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 53.22% compared to head [(`9ca43bd`)](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1370?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 53.76%.
| [Files](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1370?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...a/org/apache/uniffle/common/RemoteStorageInfo.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1370?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9SZW1vdGVTdG9yYWdlSW5mby5qYXZh) | 0.00% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1370?src=pr&el=tree&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 @@
## master #1370 +/- ##
============================================
+ Coverage 53.22% 53.76% +0.53%
+ Complexity 2717 2423 -294
============================================
Files 418 365 -53
Lines 23934 18777 -5157
Branches 2043 1809 -234
============================================
- Hits 12740 10095 -2645
+ Misses 10409 8045 -2364
+ Partials 785 637 -148
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1370?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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1364] feat(client): introduce option to control whether to use local hadoop conf for remote storage by default [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #1370:
URL: https://github.com/apache/incubator-uniffle/pull/1370#discussion_r1426425985
##########
common/src/main/java/org/apache/uniffle/common/config/RssClientConf.java:
##########
@@ -142,4 +142,12 @@ public class RssClientConf {
.enumType(ClientType.class)
.defaultValue(ClientType.GRPC)
.withDescription("Supports GRPC, GRPC_NETTY");
+
+ public static final ConfigOption<Boolean> RSS_CLIENT_REMOTE_STORAGE_USE_LOCAL_CONF_ENABLED =
+ ConfigOptions.key("rss.client.remote.storage.use-local-conf.enabled")
Review Comment:
Got 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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1364] feat(client): introduce option to control whether to use local hadoop conf for remote storage by default [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1370:
URL: https://github.com/apache/incubator-uniffle/pull/1370#discussion_r1425586987
##########
common/src/main/java/org/apache/uniffle/common/config/RssClientConf.java:
##########
@@ -142,4 +142,12 @@ public class RssClientConf {
.enumType(ClientType.class)
.defaultValue(ClientType.GRPC)
.withDescription("Supports GRPC, GRPC_NETTY");
+
+ public static final ConfigOption<Boolean> RSS_CLIENT_REMOTE_STORAGE_USE_LOCAL_CONF_ENABLED =
+ ConfigOptions.key("rss.client.remote.storage.use-local-conf.enabled")
Review Comment:
Could we use 4 levels + camel naming style? This is the style of Spark. This will avoid too many levels.
For example:
`rss.client.remote.storage.useLocalConfAsDefault`.
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1364] feat(client): introduce option to control whether to use local hadoop conf for remote storage by default [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston merged PR #1370:
URL: https://github.com/apache/incubator-uniffle/pull/1370
--
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@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org