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

[#983] improvement(tez): Optimize tez client delivery configuration #985

Merged
merged 2 commits into from
Jul 4, 2023

Conversation

zhengchenyu
Copy link
Collaborator

@zhengchenyu zhengchenyu commented Jul 3, 2023

What changes were proposed in this pull request?

Three improvement about configuration will be done in this issue.

  • 1 For now, tez client use rss_conf.xml to delivery configuration. As [#965] feat(tez): support remote shuffle for tez framework #966 is applied, we can delivery configuration by edge conf.
  • 2 delivery dynamic configuration from coordinator, then override the tez client configuration.
  • 3 delivery configuration from client side.

Why are the changes needed?

    1. rss_conf.xml is unnecessary.
    1. dynamic configuration from coordinator are not applied.
    1. config in client side can not delivery to input/ouput.

How was this patch tested?

integration test, unit test, test in yarn cluster, test in tez local mode.

@jerqi jerqi requested a review from lifeSo July 3, 2023 02:11
@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2023

Codecov Report

Merging #985 (32f83de) into master (f622aca) will increase coverage by 1.35%.
The diff coverage is 64.47%.

@@             Coverage Diff              @@
##             master     #985      +/-   ##
============================================
+ Coverage     53.22%   54.58%   +1.35%     
- Complexity     2507     2512       +5     
============================================
  Files           376      356      -20     
  Lines         20535    18161    -2374     
  Branches       1758     1764       +6     
============================================
- Hits          10930     9913    -1017     
+ Misses         8920     7630    -1290     
+ Partials        685      618      -67     
Impacted Files Coverage Δ
...on/shuffle/orderedgrouped/RssShuffleScheduler.java 45.20% <0.00%> (-0.07%) ⬇️
.../library/output/RssOrderedPartitionedKVOutput.java 48.21% <0.00%> (+2.83%) ⬆️
...z/runtime/library/output/RssUnorderedKVOutput.java 48.64% <0.00%> (+2.88%) ⬆️
...ibrary/output/RssUnorderedPartitionedKVOutput.java 48.64% <0.00%> (+2.88%) ⬆️
...ain/java/org/apache/tez/common/UmbilicalUtils.java 19.04% <50.00%> (-12.96%) ⬇️
...n/java/org/apache/tez/dag/app/RssDAGAppMaster.java 26.77% <68.42%> (+9.61%) ⬆️
...c/main/java/org/apache/tez/common/RssTezUtils.java 58.58% <75.00%> (+2.26%) ⬆️
.../main/java/org/apache/tez/common/RssTezConfig.java 92.85% <100.00%> (+1.19%) ⬆️
...rg/apache/tez/dag/app/TezRemoteShuffleManager.java 68.80% <100.00%> (+0.28%) ⬆️

... and 24 files with indirect coverage changes

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

@jerqi
Copy link
Contributor

jerqi commented Jul 3, 2023

Why are the changes needed?

    1. rss_conf.xml is unnecessary.
    1. dynamic configuration from shuffle server are not applied.

Should shuffle server be changed to coordinator?

@zhengchenyu
Copy link
Collaborator Author

Why are the changes needed?

    1. rss_conf.xml is unnecessary.
    1. dynamic configuration from shuffle server are not applied.

Should shuffle server be changed to coordinator?

OK! modification is Done!

}

public static Configuration filterRssConf(Configuration extraConf) {
Configuration conf = new Configuration();
Copy link
Contributor

Choose a reason for hiding this comment

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

先清空默认配置conf.clear()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great catch! I do it like below. It is more clear.

Configuration conf = new Configuration(false);

Copy link
Contributor

Choose a reason for hiding this comment

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

I think still need to call conf.clear(). Even if Configuration(false), will still load some configurations

Copy link
Contributor

Choose a reason for hiding this comment

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

After test, it works without clear

@jerqi jerqi changed the title [#983] [Improvement] [tez] Optimize tez client delivery configuration [#983] improvement(tez): Optimize tez client delivery configuration Jul 3, 2023
@jerqi jerqi requested a review from baitian77 July 3, 2023 11:00
Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @zhengchenyu @baitian77

@jerqi jerqi merged commit 3bd2315 into apache:master Jul 4, 2023
@zhengchenyu zhengchenyu deleted the tez.conf.improvement branch July 4, 2023 03:20
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.

4 participants