Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding JVM GC Time and Executor CPU time heuristic #279

Closed
wants to merge 291 commits into from

Conversation

skakker
Copy link
Contributor

@skakker skakker commented Aug 23, 2017

The ratio of jvmGcTime to executorCpuTime is checked, to see if GC is taking too much time (providing more memory could help) or too little time (memory may be over provisioned, and can be reduced).

Its recommended to increase the executor memory if too much time is being spent in GC:
Low: avg (jvmGcTime / executorCpuTime) >= .08
Moderate: avg (jvmGcTime / executorCpuTime) >= .1
Critical: avg (jvmGcTime / executorCpuTime) >= .15
Severe:avg (jvmGcTime / executorCpuTime) >= .2

Its recommended to decrease the executor memory if too little time is being spent in GC.
Low: avg (jvmGcTime / executorCpuTime) < .05)
Moderate: avg (jvmGcTime / executorCpuTime) < .04)
Critical: avg (jvmGcTime / executorCpuTime) < .03)
Severe: avg (jvmGcTime / executorCpuTime) < .01)

mcvsubbu and others added 30 commits January 16, 2015 13:53
hadoop-1 does not have JobStatus.getFinishTime(). This causes dr-elephant to hang.

Set the start time to be same as finish time for h1 jobs.

For consistency, reverted to the old method of scraping the job tracker url so that we get only
start time, and set the finish time to be equal to start time for retired jobs as well.

RB=417975
BUGS=HADOOP-8640
R=fli,mwagner
A=fli
RB=417448
BUGS=HADOOP-8648
R=fli
A=fli
…increasing mapred.min.split.size for too many mappers, NOT mapred.max.split.size
…name

RB=468832
BUGS=HADOOP-10405
R=fli
A=fli,ahsu
…JobType; 2. use only application id all data holders and use job id while saving to DB; 3. Change configuration styles of heuristics, fetchers and jobtypes into separate files; 4. Make those configuration files providable via other locations so that we can seemlessly change configuration without releasing a new deployment zip. 5. Remove HadoopVersion classic, yarn, using 1 or 2 instead.
executorRunTime: Long,
name: String
): StageDataImpl = new StageDataImpl(
status: StageStatus,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the indentation.

it("has the total executor cpu time") {
evaluator.executorCpuTime should be(18600000)
}
it("has ascending severity for ratio of JVM GC time to executor cpu time") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please maintain a spacing after each assertion.

@@ -116,6 +129,15 @@ object StagesHeuristic {
critical = Duration("60min").toMillis,
ascending = true
)

/** The severity thresholds for the ratio of JVM GC Time and executor CPU Time */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment on what basis these thresholds were set? If it is experimental, please mention it in the comments and that it may change in the future.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are experimental, so are likely to change as we evaluate the results on more Spark applications.

stageDatas: Seq[StageDataImpl],
appConfigurationProperties: Map[String, String]
): SparkApplicationData = {
stageDatas: Seq[StageDataImpl],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix indentation

resultDetails = resultDetails :+ new HeuristicResultDetails("Note", "The ratio of JVM GC Time and executor Time is above normal, we recommend to increase the executor memory")
}
if (evaluator.severityTimeD.getValue != Severity.NONE.getValue) {
resultDetails = resultDetails :+ new HeuristicResultDetails("Note", "The ratio of JVM GC Time and executor Time is below normal, we recommend to decrease the executor memory")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify this behavior in the comments?

@@ -36,7 +36,8 @@ import org.apache.spark.status.api.v1.StageStatus
* each stage.
*/
class StagesHeuristic(private val heuristicConfigurationData: HeuristicConfigurationData)
extends Heuristic[SparkApplicationData] {
extends Heuristic[SparkApplicationData] {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the change in indentation intentional?

@@ -116,6 +129,15 @@ object StagesHeuristic {
critical = Duration("60min").toMillis,
ascending = true
)

/** The severity thresholds for the ratio of JVM GC Time and executor CPU Time */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are experimental, so are likely to change as we evaluate the results on more Spark applications.

@@ -196,15 +233,44 @@ object StagesHeuristic {
val averageExecutorRuntime = stageData.executorRunTime / executorInstances
(averageExecutorRuntime, stageRuntimeMillisSeverityThresholds.severityOf(averageExecutorRuntime))
}

private def getTimeValues(stageDatas: Seq[StageData]): (Long, Long) = {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to add some comments for the function.

@@ -179,6 +179,7 @@ trait TaskData{
trait TaskMetrics{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be useful to have totalCpuTime in ExecutorSummary (it would be fewer calls to get the info)? It seems good to have at the stage level, so that users know which stage is problematical.

@shkhrgpt
Copy link
Contributor

shkhrgpt commented Oct 5, 2017

I think it would good if you can provide a brief description of this PR. It helps in the review.
Thanks.

@edwinalu
Copy link

Thanks for adding StageStatus. The new changes look good.

@@ -0,0 +1,17 @@
package com.linkedin.drelephant.spark.fetchers.statusapiv1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a Java class in a Scala package looks weird. Either create a Scala class here or create a separate package for this class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In apache spark documentation, StageStatus is a .java class hence, to keep things uniform, I will create a new package for this class.

Exception.ignoring(classOf[NoSuchElementException]) {
stageDatas.foreach((stageData: StageData) => {
stageData.tasks.get.values.foreach((taskData: TaskData) => {
jvmGcTimeTotal += taskData.taskMetrics.getOrElse(taskMetricsDummy).jvmGcTime
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's correct to add GC time for each task to get a total GC time. JVM GC time is a global entity if multiple tasks are running in parallel, they will have the same GC time, and adding them up will be overcounting.

var (jvmTime, executorCpuTime) = getTimeValues(stageDatas)

var ratio: Double = {
ratio = jvmTime.toDouble / executorCpuTime.toDouble
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jvm GC time is in wall clock, and Executor CPU time is in CPU clock. I don't know if dividing them is right to do here. Instead of using executor CPU time, we should executor duration.

shuffleWriteMetrics = None)).get
//ignoring the exception as there are cases when there is no task data, in such cases, 0 is taken as the default value
Exception.ignoring(classOf[NoSuchElementException]) {
stageDatas.foreach((stageData: StageData) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think makes sense to sum these ratios over stages, we should sum them over executors.

@shkhrgpt
Copy link
Contributor

Why this heuristic is part of stage heuristic. Since JVM GC is an executor property, I think it makes more sense to see this as executor heuristic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.