[#1554] feat(spark): Fetch dynamic client conf as early as possible#1557
Conversation
| conf.set(RssSparkConfig.RSS_DYNAMIC_CLIENT_CONF_ENABLED.key(), "false"); | ||
| conf.set(RssSparkConfig.RSS_ACCESS_ID.key(), "mockId"); | ||
| conf.set(RssSparkConfig.RSS_ENABLED.key(), "true"); | ||
| conf.set(RssSparkConfig.RSS_STORAGE_TYPE.key(), "MEMORY_LOCALFILE"); |
There was a problem hiding this comment.
This test further down asserts an exception is thrown. Moving RssSparkShuffleUtils.validateRssClientConf(sparkConf) up in the RssShuffleManager constructor throws a different exception if the storage type is not set.
There was a problem hiding this comment.
Coordinator can apply the storage type to the client. Will this pull request affect this feature.
There was a problem hiding this comment.
This test calls into new RssShuffleManager(sparkConf, false), which mimics creating the shuffle manager on an executor, and executors do not fetch dynamic config, so the given sparkConf must have the storage type set for the test.
The logic of applying config is unchanged, this only happens earlier. I presume there are tests for this use case, which should check the desired behaviour is preserved.
|
And I think the PR title is not proper for this change, which don't reflect the feature of overwriting the existing client conf. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1557 +/- ##
============================================
+ Coverage 53.96% 54.94% +0.98%
- Complexity 2858 2862 +4
============================================
Files 438 418 -20
Lines 24793 22455 -2338
Branches 2109 2111 +2
============================================
- Hits 13379 12339 -1040
+ Misses 10576 9348 -1228
+ Partials 838 768 -70 ☔ View full report in Codecov by Sentry. |
this is not new, this existed before |
EnricoMi
force-pushed
the
dynamic-client-conf
branch
from
b687162 to
5d5cd9d
Compare
EnricoMi
force-pushed
the
dynamic-client-conf
branch
from
187ed27 to
d97f826
Compare
|
Could you help resolve the conflict? @EnricoMi |
EnricoMi
force-pushed
the
dynamic-client-conf
branch
from
d97f826 to
cc564bb
Compare
EnricoMi
force-pushed
the
dynamic-client-conf
branch
from
cc564bb to
33ac575
Compare
4 participants
What changes were proposed in this pull request?
Fetches dynamic client config as early as possible, to be able to use dynamic client config to create the shuffle client with updated config.
Why are the changes needed?
Providing config or the shuffle client via coordinator is operationally useful as cluster-wide settings can be deployed through the cluster and changed over time. Clients and apps do not need to change configs.
Fixes Spark part of #1554
Does this PR introduce any user-facing change?
More configs can be provided via coordinators.
How was this patch tested?
Existing and follow-up unit tests.