Skip to content

Conversation

@maropu
Copy link
Member

@maropu maropu commented Mar 11, 2019

What changes were proposed in this pull request?

This pr added code to log ignored hints in RemoveAllHints.

How was this patch tested?

Pass Jenkins.

@maropu
Copy link
Member Author

maropu commented Mar 11, 2019

@gatorsmile Does this pr matches your intention in the Jira?

@SparkQA
Copy link

SparkQA commented Mar 11, 2019

Test build #103324 has finished for PR 24055 at commit ddc4f85.

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

@dongjoon-hyun
Copy link
Member

Retest this please

@SparkQA
Copy link

SparkQA commented Mar 16, 2019

Test build #103560 has finished for PR 24055 at commit ddc4f85.

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

@maropu
Copy link
Member Author

maropu commented Mar 19, 2019

ping @gatorsmile @maryannxue

.stringConf
.transform(_.toUpperCase(Locale.ROOT))
.checkValue(logLevel => Set("TRACE", "DEBUG", "INFO", "WARN", "ERROR").contains(logLevel),
"Invalid value for 'spark.sql.optimizer.planChangeLog.level'. Valid values are " +
Copy link
Member

Choose a reason for hiding this comment

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

spark.sql.optimizer.planChangeLog.level -> spark.sql.optimizer.planHintIgnoreLog.level?

@SparkQA
Copy link

SparkQA commented Mar 20, 2019

Test build #103698 has finished for PR 24055 at commit 82979d8.

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

@maropu
Copy link
Member Author

maropu commented Mar 20, 2019

Retest this please

@SparkQA
Copy link

SparkQA commented Mar 20, 2019

Test build #103706 has finished for PR 24055 at commit 82979d8.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@maropu
Copy link
Member Author

maropu commented Mar 20, 2019

Retest this please

@SparkQA
Copy link

SparkQA commented Mar 20, 2019

Test build #103717 has finished for PR 24055 at commit 82979d8.

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

@SparkQA
Copy link

SparkQA commented Mar 20, 2019

Test build #103714 has finished for PR 24055 at commit 82979d8.

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

@gatorsmile
Copy link
Member

cc @maryannxue @maropu @cloud-fan

.stringConf
.createOptional

val PLAN_HINT_IGNORE_LOG_LEVEL =
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really worth a config?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1. I don't think we need to make it configurable. We can just log warning.

Seq(errMsgRepa))
}

private def withLogLevel(logLevel: String)(f: => Unit): String = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we reuse SparkFunSuite.withLogAppender?

}
}

private class IgnoredHintLogger {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a new class here? Is there any reason why we can't use HintErrorLogger?

Copy link
Member Author

@maropu maropu Oct 23, 2019

Choose a reason for hiding this comment

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

Yea, HintErrorLogger can covert all the case in this pr, so I'll close this. Thanks (I didn't notice that the pr of HintErrorLogger had been merged after this pr made.

@maropu maropu closed this Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants