SPARK-3779. yarn spark.yarn.applicationMaster.waitTries config should be...

[sryza]

... changed to a time period

[sryza]

@tgravescs

[SparkQA]

Test build #23878 has started for PR 3471 at commit 12eb0cd.

• This patch merges cleanly.

[SparkQA]

Test build #23878 has finished for PR 3471 at commit 12eb0cd.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23878/
Test PASSed.

[WangTaoTheTonic]

Why we need a val waitTime = 100?

[tgravescs]

can we rename it to spark.yarn.am.waitTime? (to be consistent with pr 3409 and possibly others)

Also it might be nice to append ms so user knows its specified in milliseconds

[sryza]

Thanks for the feedback, Tom. Updated the patch to reflect your and Wang Tao's comments.

I left out adding MS to the config name because it's inconsistent with all Spark's existing configs. I agree that it would have been better to start out including the units in config names, but I think it'll be confusing to different conventions for different configs here.

[SparkQA]

Test build #24142 has started for PR 3471 at commit ce6dff2.

• This patch merges cleanly.

[SparkQA]

Test build #24142 has finished for PR 3471 at commit ce6dff2.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24142/
Test PASSed.

[tgravescs]

changes look good.

[WangTaoTheTonic]

LGTM +1

[andrewor14]

LGTM, but do we need to add backward compatibility? This patch seems to get rid of the old config altogether and Spark will silently ignore the setting if an old application configured this.

[sryza]

Updated patch uses the old property to set the wait time if the new one isn't set.

[AmplabJenkins]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24275/
Test FAILed.

[andrewor14]

retest this please

[SparkQA]

Test build #24293 has started for PR 3471 at commit 20b9887.

• This patch merges cleanly.

[andrewor14]

Hey @sryza unfortunately this will have to block until we figure out in #3409 what the appropriate naming scheme for AM configs to use collectively. Otherwise we might have two layers of deprecation, which will definitely be confusing to many users and developers.

[SparkQA]

Test build #24293 has finished for PR 3471 at commit 20b9887.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24293/
Test PASSed.

[sryza]

@andrewor14, makes sense.

[tgravescs]

test this please

[tgravescs]

this looks good. kicked jenkins to run again since last run was while ago.

[SparkQA]

Test build #24593 has started for PR 3471 at commit 20b9887.

• This patch merges cleanly.

[SparkQA]

Test build #24593 has finished for PR 3471 at commit 20b9887.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24593/
Test PASSed.

[tgravescs]

Looks good, +1