[SPARK-6014] [core][hotfix] Add try-catch block around ShutDownHook

[nishkamravi2]

Add a try/catch block around removeShutDownHook else IllegalStateException thrown in YARN cluster mode (see #4690)

cc @andrewor14, @srowen

[nishkamravi2]

See Utils.inShutDown discussion in 4690.

[nishkamravi2]

Can this be tested please. Thanks.

[vanzin]

See Utils.inShutDown discussion in 4690.

The part about it being expensive? This is hardly a performance sensitive path. It could also be made faster by first checking shutdownHooks.shutdown before trying the Runtime.blah route.

But in any case, not a big deal.

[srowen]

Can this simply not throw an exception?

[vanzin]

What would it do instead? Just ignore the call and return false?

Probably not a big deal in practice, but it does overload the return value to mean "either the hook doesn't exist, or I can't remove it at this time".

[srowen]

Yes. Asking to remove a hook always races with shutdown, so I suppose code can never guarantee it's been able to remove a hook before shutdown. It can't be sure it has successfully removed the hook before it executes. So it doesn't seem to bad to just try to remove the hook, which might or might not have executed and discarded already, and return normally either way. That is do you want to prohibit removal of a hook that hasn't run, even during shutdown? meh, might be a simpler way to resolve this.

[vanzin]

Hmm. It should be ok to allow removing the hooks during shutdown. SparkShutdownHookManager.runAll is safe against modifications to the underlying queue.

[nishkamravi2]

I would be fine with 'not explicitly throwing an exception' and keeping the try-catch block.

[vanzin]

LGTM. try..catch seems unnecessary now but doesn't hurt either.

[srowen]

Why keep the block? what exception is expected now and why is it safe to squash it (without logging even)?

[nishkamravi2]

A case of erring on the conservative side: the implementation of Utils.removeShutdownHook could change over time. The invocation from DiskBlockManager should not have to rely on it being exception-free.

[nishkamravi2]

With regards to logging, I thought you had recommended not logging (in 4690) . Same reasons apply?

[srowen]

There I was talking about a normal, expected exception. If this method changes to not throw an exception that we think of as normal and expect-able, then whatever this catches is surprising and should be logged. By this logic we should catch a lot of exceptions in a lot of places... this makes me believe that the author expected a particular exception and specially knows it can be silently ignored. That doesn't seem to be the argument anymore.

[nishkamravi2]

The original PR in 4690 didn't make assumptions about the type of Exception thrown and logged it, I guess we were thinking slightly differently back then too. Agreed: generalizing and logging seems like the best way forward.

[srowen]

In general I'd argue for not just catching exceptions broadly as a defense. This is a stop() method though, and often the right behavior is to just keep going even if stopping failed due to some I/O problem or messed up state. Here I don't see what exception this method can throw. I'd still find try-catch surprising and would not expect it.

[vanzin]

Since we're talking exceptions, SparkShutdownHookManager.runAll could be modified to catch exceptions for individual hooks and ignore them. It already logs them, but logUncaughtExceptions re-throws, so an exception would cause downstream hooks to not be executed.

[nishkamravi2]

@srowen I think I'd approach this one as why-not (especially with logging enabled). This particular call site has warranted two separate PRs attempting to add a try-catch block because the code in Utils changed and thus stands out from the rest of Spark code in that regard?

[nishkamravi2]

@vanzin Are we suggesting adding a try-catch block inside the while loop in runAll or deleting throw's in logUncaughtExceptions ?

[vanzin]

@nishkamravi2 just adding Try() around the logUncaughtExceptions call should be sufficient.

[nishkamravi2]

While we are at it: any particular reason for using PriorityQueue or can we replace it by ConcurrentLinkedQueue or something and get rid of a few synchronized's ?

[vanzin]

Hooks have a priority so that they can choose the order they're executed in. e.g. if some hook really needs to execute before another one, that's possible. Similar to how the Spark hook needs to execute before the HDFS one to avoid exceptions.

The locking shouldn't cause issues - this is not a hot path where avoiding locks brings any benefit. Outside of shutdown, there's just a handful of places where the methods are called. During shutdown, everything happens from runAll, so any re-entrant calls already have the lock held and thus can go through.

[nishkamravi2]

@vanzin Sounds reasonable.

[srowen]

@nishkamravi2 this isn't the same method or exception handling as before. The question is what's right to do given the current state of the code. It feels a little arbitrary to wrap this invocation but not others, but it's a stop() method and at least logs it, so won't argue it further.

[srowen]

ok to test

[SparkQA]

Test build #701 has started for PR 5672 at commit 0f1abd0.

[srowen]

This test actually succeeded normally, just wasn't able to post back here. Going ahead.