Updates Spark configuration heuristic severity calculations #229

shkhrgpt · 2017-03-29T22:55:37Z

As discussed in document, updating the spark configuration heuristic. The main changes are based on the following logics:

Serializer: Suggest using Kryo serializer because its higher performance than the Java Serializer. Maybe a moderate severity if not using Kryo. Config key spark.serializer should be set to org.apache.spark.serializer.KryoSerializer
Shuffle Service: If not set -> Moderate -> Suggest configuring a Shuffle service so that jobs can survive an executor failure. spark.shuffle.service.enabled should be set to true
If Spark Dynamic Allocation is on, and no Shuffle Service configured then Severe severity.

Also updates unit tests.

@shankar37 @akshayrai @superbobry

superbobry · 2017-03-30T00:07:02Z

app/com/linkedin/drelephant/spark/heuristics/ConfigurationHeuristic.scala

@@ -128,18 +135,29 @@ object ConfigurationHeuristic {
    lazy val serializer: Option[String] = getProperty(SPARK_SERIALIZER_KEY)

    lazy val serializerSeverity: Severity = serializer match {
-      case None => Severity.NONE
+      case None => Severity.MODERATE
      case Some(`serializerIfNonNullRecommendation`) => Severity.NONE


I know this is not part of your PR, but it looks suspicious nonetheless: IIUC the third branch is unreachable because the pattern Some(`foo`) is equavalent to Some(_) modulo variable name.

scala> Some(42) match { | case Some(`foo`) => "first" | case Some(_) => "second" | } res1: String = first

I don't know if that's true, otherwise the unit tests will fail. I think it all depends on the value of foo. I tried the code locally, and got the following outputs:

scala> val foo = 50 foo: Int = 50 scala> Some(42) match { | case Some(`foo`) => "first" | case Some(_) => "second" | } res1: String = second scala> val foo = 42 foo: Int = 42 scala> Some(42) match { | case Some(`foo`) => "first" | case Some(_) => "second" | } res2: String = first

I am going to leave this code unchanged.

Oh, evil Scala...

superbobry · 2017-03-30T00:12:04Z

app/com/linkedin/drelephant/spark/heuristics/ConfigurationHeuristic.scala

+      case Some(_) => DEFAULT_SERIALIZER_IF_NON_NULL_SEVERITY_IF_RECOMMENDATION_UNMET
+    }
+
+    lazy val isDynamicAllocationEnabled: Option[Boolean] = getProperty(SPARK_DYNAMIC_ALLOCATION_ENABLED).map(_.toBoolean)


Is there a reason for not applying defaults at declaration site? I might be wrong, but it looks like in the following match statement you treat None as false for both properties.

Here's a rewritten match:

lazy val shuffleAndDynamicAllocationSeverity = (isDynamicAllocationEnabled, isShuffleServiceEnabled) match { case (_, Some(true)) | (None, Some(true)) => Severity.NONE case (Some(true), Some(false)) | (Some(true), None) => Severity.SEVERE case (Some(false), Some(false)) => Severity.MODERATE case (Some(false), None) => Severity.MODERATE case (None, Some(false)) => Severity.MODERATE case (None, None) => Severity.MODERATE }

The first case is essentially

case (_, true) => Severity.None

the next bunch is

case (true, false) => Severity.SEVERE

and finally

case (false, false) => Severity.MODERATE

Thanks for the suggestion of using default value instead of None. It simplified the pattern matching.

superbobry · 2017-03-30T00:13:40Z

app/com/linkedin/drelephant/spark/heuristics/ConfigurationHeuristic.scala

+    lazy val isDynamicAllocationEnabled: Option[Boolean] = getProperty(SPARK_DYNAMIC_ALLOCATION_ENABLED).map(_.toBoolean)
+    lazy val isShuffleServiceEnabled: Option[Boolean] = getProperty(SPARK_SHUFFLE_SERVICE_ENABLED).map(_.toBoolean)
+
+    lazy val shuffleAndDynamicAllocationSeverity = (isDynamicAllocationEnabled, isShuffleServiceEnabled) match {


I think it could be useful to have a brief comment here outlining the rationale. The reader could of course trace it back to that commit, but a comment is more reader-friendly in my opinion.

superbobry · 2017-03-30T00:15:08Z

I don't have an opinion on dynamic allocation/shuffle service, but the Kryo heuristic looks very useful 👍 .

shkhrgpt · 2017-03-30T01:56:29Z

Thanks @superbobry for the review.

shankar37 · 2017-04-10T09:07:05Z

app/com/linkedin/drelephant/spark/heuristics/ConfigurationHeuristic.scala

@@ -77,6 +74,14 @@ class ConfigurationHeuristic(private val heuristicConfigurationData: HeuristicCo
      new HeuristicResultDetails(


Add recommendations to the heuristic details. If multiple of them fails, we take the max and user has to fix one to know there is more to fix.

@shankar37 I did not understand this comment. Where should I add recommendations to the heuristic details?

For all configs that we are checking, we should add a detail like of key-value pair like

whenever we find that check not yielding none.
thanks
shankar

@shankar37 I am sorry, I still don't understand your comment. Can you please give an example?
Thank you.

@shankar37 Can you please provide an example here. I am not able to understand your comment. Thank you.

Sorry, missed your reply. An example would be

if( serializer check fails)
HeuristicResult.add( new HeuristicResultDetails(
"Serializer",
"KyroSerializer Not Enabled.") )
if( dynamicallocation check fails) {
HeuristicResult.add (
new HeuristicResultDetails(
"DynamicAllocation without Shuffle",
"DynamicAllocation is enabled but."))
}

I don't understand why do we need to have an if statement here, we don't have it in any other case. I am already adding details like the following:

new HeuristicResultDetails(
SPARK_DYNAMIC_ALLOCATION_ENABLED,
formatProperty(evaluator.isDynamicAllocationEnabled.map(.toString))
),
new HeuristicResultDetails(
SPARK_SHUFFLE_SERVICE_ENABLED,
formatProperty(evaluator.isShuffleServiceEnabled.map(.toString))
)

Here is the scenario. If I have a job that fails both dynamic allocation check and kyroserializer check you will take the max of severity and show that. Let's say that is dynamic allocation check. Once that is fixed, they will rerun the job and find it failing the kyroserializer check now. I am asking to make it obvious to the user they have multiple issues to fix within this one heuristic.

@shankar37 I think I got it. Thank you very much. I've updated the PR. Plese have a look.

akshayrai · 2017-04-13T09:01:20Z

+1 LGTM

… fix-config-heuristic

akshayrai · 2017-04-20T05:24:54Z

Thanks @shkhrgpt.

…#229)

Updates Spark configuration heuristic severity calculations

2304b4d

superbobry reviewed Mar 30, 2017

View reviewed changes

Replaces None with false and adds comments

3d54d8a

shkhrgpt added 2 commits March 29, 2017 18:58

Fixes typos in comments

b0f59e0

Adds more comments

e380e8f

shankar37 approved these changes Apr 10, 2017

View reviewed changes

superbobry approved these changes Apr 13, 2017

View reviewed changes

shkhrgpt added 3 commits April 18, 2017 21:56

Merge branch 'master' of https://github.com/linkedin/dr-elephant into…

036d1ee

… fix-config-heuristic

Merge branch 'master' of https://github.com/linkedin/dr-elephant into…

ae38cd8

… fix-config-heuristic

Adds conditional heuristic result details

714856e

akshayrai merged commit 7c373d4 into linkedin:master Apr 20, 2017

skakker pushed a commit to skakker/dr-elephant that referenced this pull request Dec 14, 2017

Updates Spark configuration heuristic severity calculations (linkedin…

0f2a38a

…#229)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates Spark configuration heuristic severity calculations #229

Updates Spark configuration heuristic severity calculations #229

shkhrgpt commented Mar 29, 2017

superbobry Mar 30, 2017

shkhrgpt Mar 30, 2017 •

edited

Loading

superbobry Mar 30, 2017

superbobry Mar 30, 2017

shkhrgpt Mar 30, 2017

superbobry Mar 30, 2017

shkhrgpt Mar 30, 2017

superbobry commented Mar 30, 2017

shkhrgpt commented Mar 30, 2017

shankar37 Apr 10, 2017

shkhrgpt Apr 12, 2017

shankar37 Apr 14, 2017

shkhrgpt Apr 14, 2017

shkhrgpt Apr 18, 2017

shankar37 Apr 18, 2017

shkhrgpt Apr 18, 2017

shankar37 Apr 19, 2017

shkhrgpt Apr 20, 2017

akshayrai commented Apr 13, 2017

akshayrai commented Apr 20, 2017

		@@ -77,6 +74,14 @@ class ConfigurationHeuristic(private val heuristicConfigurationData: HeuristicCo
		new HeuristicResultDetails(

Updates Spark configuration heuristic severity calculations #229

Updates Spark configuration heuristic severity calculations #229

Conversation

shkhrgpt commented Mar 29, 2017

Choose a reason for hiding this comment

shkhrgpt Mar 30, 2017 • edited Loading

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

superbobry commented Mar 30, 2017

shkhrgpt commented Mar 30, 2017

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

akshayrai commented Apr 13, 2017

akshayrai commented Apr 20, 2017

shkhrgpt Mar 30, 2017 •

edited

Loading