[SPARK-3410] The priority of shutdownhook for ApplicationMaster should not be integer literal

[sarutak]

I think, it need to keep the priority of shutdown hook for ApplicationMaster than the priority of shutdown hook for o.a.h.FileSystem depending on changing the priority for FileSystem.

[SparkQA]

QA tests have started for PR 2283 at commit 717aba2.

• This patch merges cleanly.

[srowen]

This change makes this shutdown hook lower than FileSystem's, whereas it used to be higher. Also does this compile for yarn-alpha too? Given the time it went in, it probably works with all supported Hadoop versions but worth checking.

[sarutak]

@srowen It's confused but lower value is higher priority.

[srowen]

Ah I see. That's fine, I just wasn't sure which the intent was since I think the original description is missing a word.

[SparkQA]

QA tests have finished for PR 2283 at commit 717aba2.

• This patch passes unit tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • case class AddJar(path: String) extends LeafNode with Command

[sarutak]

Ah, sorry it's my wrong. I confirm the logic of ShutdownHookManager, and higher value is higher priority.

[sarutak]

test this please.

[sarutak]

Jenkins, retest this please .

[tgravescs]

I don't think this is really necessary as I see the value of the Filesystem one as a public api now and changing its value would break compatibility, but I'm ok with it. Yes yarn-alpha has this defined.

Higher value is higher priority. I would rather leave it at value 30 or at least some priorities in between, so I would rather see + 20. 30 is also what mapreduce uses so if Hadoop would to add others in we would be better off to imitate MR.

[tgravescs]

@sarutak can you please either upmerge or close this

[sarutak]

Thank you for notification. I've rebased.

[SparkQA]

QA tests have started for PR 2283 at commit 2f0aee3.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2283 at commit 2f0aee3.

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

[tgravescs]

thanks, can you please also address my concerns in the comment above.

[sarutak]

@tgravescs I agree with +20, and how about asserting the priority is between HDFS's and MR's?

[SparkQA]

QA tests have started for PR 2283 at commit 54eb68f.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2283 at commit 54eb68f.

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

[tgravescs]

@sarutak an assertion is a good idea. Could you change the priority back to 30 instead of 20? Also instead of having 20 hardcoded in 2 different places can you make one static for this something like:

val SHUTDOWN_HOOK_PRIORITY: int = 30

in the ApplicationMaster object area.

[sarutak]

@tgravescs Oh, I mistook. The priority is duplicated.
Now I've modified. Thanks!

[SparkQA]

QA tests have started for PR 2283 at commit e8ce90b.

• This patch merges cleanly.

[SparkQA]

QA tests have started for PR 2283 at commit ce86eb9.

• This patch merges cleanly.

[SparkQA]

QA tests have started for PR 2283 at commit bd6cc53.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2283 at commit e8ce90b.

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

[SparkQA]

QA tests have finished for PR 2283 at commit ce86eb9.

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

[SparkQA]

QA tests have finished for PR 2283 at commit bd6cc53.

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

[SparkQA]

QA tests have started for PR 2283 at commit 1d44fef.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2283 at commit 1d44fef.

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

[tgravescs]

+1 looks good. Thanks @sarutak