[KYUUBI #5766] Default `spark.yarn.maxAppAttempts` to 1 for spark engine #5798

wForget · 2023-12-01T05:37:51Z

🔍 Description

Issue References 🔗

This pull request fixes #5766

Describe Your Solution 🔧

As discussed in #5766 (comment), we should add spark.yarn.maxAppAttempts=1 for spark engine when spark.master is yarn.

Types of changes 🔖

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests

Checklists

📝 Author Self Checklist

My code follows the style guidelines of this project
I have performed a self-review
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
This patch was not authored or co-authored using Generative Tooling

📝 Committer Pre-Merge Checklist

Be nice. Be informative.

codecov-commenter · 2023-12-01T07:13:16Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (51c3e03) 61.29% compared to head (6477dfd) 61.27%.
Report is 2 commits behind head on master.

Files	Patch %	Lines
...ache/kyuubi/engine/spark/SparkProcessBuilder.scala	75.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #5798      +/-   ##
============================================
- Coverage     61.29%   61.27%   -0.03%     
  Complexity       23       23              
============================================
  Files           608      608              
  Lines         36027    36033       +6     
  Branches       4952     4952              
============================================
- Hits          22083    22078       -5     
- Misses        11556    11559       +3     
- Partials       2388     2396       +8

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

cxzl25 · 2023-12-01T11:21:05Z

kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala

+  def extraYarnConf: Map[String, String] = {
+    if (clusterManager().exists(_.toLowerCase(Locale.ROOT).startsWith("yarn"))) {
+      // set `spark.yarn.maxAppAttempts` to 1 to avoid invalid attempts.
+      Map(YARN_MAX_APP_ATTEMPTS_KEY -> "1")


YARN-5617. AMs only intended to run one attempt can be run more than once

Fix Version/s: 2.9.0, 3.0.0-alpha4, 2.8.2

maybe we should add such info to the comment

This seems to be a very early issue, do we still need to add relevant comment to code?

would be better to leave a hint for things happened after Hadoop 2.7

…rk engine

cxzl25 · 2023-12-06T04:21:17Z

kyuubi-server/src/main/scala/org/apache/kyuubi/engine/spark/SparkProcessBuilder.scala

@@ -140,8 +140,9 @@ class SparkProcessBuilder(
      allConf = allConf ++ zkAuthKeytabFileConf(allConf)
    }
    // pass spark engine log path to spark conf
-    (allConf ++ engineLogPathConf ++ appendPodNameConf(allConf)).foreach { case (k, v) =>
-      buffer ++= confKeyValue(convertConfigKey(k), v)
+    (allConf ++ engineLogPathConf ++ extraYarnConf ++ appendPodNameConf(allConf)).foreach {


It looks like extraYarnConf will override the user's configuration.

I have moved extraYarnConf to the front

Emm... if we really want to respect user's conf here, we need to do things like appendPodNameConf does

thanks, fixed

The construction of allConf is too complex, I prefer to put kvs into a mutable map, let's optimize it later.

# 🔍 Description ## Issue References 🔗 This pull request fixes #5766 ## Describe Your Solution 🔧 As discussed in #5766 (comment), we should add `spark.yarn.maxAppAttempts=1` for spark engine when `spark.master` is `yarn`. ## Types of changes 🔖 - [X] Bugfix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Test Plan 🧪 #### Behavior Without This Pull Request ⚰️ #### Behavior With This Pull Request 🎉 #### Related Unit Tests --- # Checklists ## 📝 Author Self Checklist - [x] My code follows the [style guidelines](https://kyuubi.readthedocs.io/en/master/contributing/code/style.html) of this project - [x] I have performed a self-review - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html) ## 📝 Committer Pre-Merge Checklist - [x] Pull request title is okay. - [x] No license issues. - [x] Milestone correctly set? - [ ] Test coverage is ok - [x] Assignees are selected. - [x] Minimum number of approvals - [x] No changes are requested **Be nice. Be informative.** Closes #5798 from wForget/KYUUBI-5766-2. Closes #5766 6477dfd [wforget] fix c50f656 [wforget] fix order dbc1891 [wforget] comment a493e29 [wforget] fix style 4fa0651 [wforget] fix test b899646 [wforget] add test 954a30d [wforget] [KYUUBI #5766] Default `spark.yarn.maxAppAttempts` to 1 for spark engine Authored-by: wforget <[email protected]> Signed-off-by: wforget <[email protected]> (cherry picked from commit 6a282fc) Signed-off-by: wforget <[email protected]>

wForget · 2023-12-07T12:13:05Z

Thanks, merged to master and 1.8

github-actions bot added the module:server label Dec 1, 2023

cxzl25 reviewed Dec 1, 2023

View reviewed changes

pan3793 approved these changes Dec 5, 2023

View reviewed changes

wForget added 4 commits December 5, 2023 19:26

[KYUUBI apache#5766] Default spark.yarn.maxAppAttempts to 1 for spa…

954a30d

…rk engine

add test

b899646

fix test

4fa0651

fix style

a493e29

wForget force-pushed the KYUUBI-5766-2 branch from 49b78fe to a493e29 Compare December 5, 2023 11:40

comment

dbc1891

wForget requested review from pan3793 and cxzl25 December 6, 2023 03:01

pan3793 approved these changes Dec 6, 2023

View reviewed changes

cxzl25 reviewed Dec 6, 2023

View reviewed changes

wForget added 2 commits December 6, 2023 12:48

fix order

c50f656

fix

6477dfd

pan3793 approved these changes Dec 6, 2023

View reviewed changes

cxzl25 approved these changes Dec 7, 2023

View reviewed changes

wForget self-assigned this Dec 7, 2023

wForget mentioned this pull request Dec 7, 2023

[TASK][EASY][SPARK] kyuubi.engine.submit.time is wrong for ApplicationMaster retries #5766

Closed

4 tasks

wForget added this to the v1.8.1 milestone Dec 7, 2023

wForget closed this in 6a282fc Dec 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KYUUBI #5766] Default `spark.yarn.maxAppAttempts` to 1 for spark engine #5798

[KYUUBI #5766] Default `spark.yarn.maxAppAttempts` to 1 for spark engine #5798

wForget commented Dec 1, 2023 •

edited

Loading

codecov-commenter commented Dec 1, 2023 •

edited

Loading

cxzl25 Dec 1, 2023

pan3793 Dec 5, 2023

wForget Dec 5, 2023

pan3793 Dec 5, 2023

cxzl25 Dec 6, 2023

wForget Dec 6, 2023

pan3793 Dec 6, 2023

wForget Dec 6, 2023

wForget Dec 6, 2023

wForget commented Dec 7, 2023

[KYUUBI #5766] Default spark.yarn.maxAppAttempts to 1 for spark engine #5798

[KYUUBI #5766] Default spark.yarn.maxAppAttempts to 1 for spark engine #5798

Conversation

wForget commented Dec 1, 2023 • edited Loading

🔍 Description

Issue References 🔗

Describe Your Solution 🔧

Types of changes 🔖

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests

Checklists

📝 Author Self Checklist

📝 Committer Pre-Merge Checklist

codecov-commenter commented Dec 1, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wForget commented Dec 7, 2023

[KYUUBI #5766] Default `spark.yarn.maxAppAttempts` to 1 for spark engine #5798

[KYUUBI #5766] Default `spark.yarn.maxAppAttempts` to 1 for spark engine #5798

wForget commented Dec 1, 2023 •

edited

Loading

codecov-commenter commented Dec 1, 2023 •

edited

Loading