Skip to content

Conversation

@jzhuge
Copy link
Member

@jzhuge jzhuge commented Aug 6, 2019

What changes were proposed in this pull request?

LookupCatalog's sessionCatalog and defaultCatalog logs an error message and the exception stack upon any nonfatal exception. When either catalog is misconfigured, this may clutter the console and alarm the user unnecessarily. It should be enough to print a warning and return None.

How was this patch tested?

Manual test cases

Start Spark shell with either of these configurations:

  • spark.sql.catalog.session=noclass
  • spark.sql.default.catalog=def
  • spark.sql.default.catalog=def and spark.sql.catalog.def=noclass

Enter spark.sessionState.analyzer.defaultCatalog or spark.sessionState.analyzer.sessionCatalog at the prompt, expect a warning and no stack trace.

@SparkQA
Copy link

SparkQA commented Aug 7, 2019

Test build #108733 has finished for PR 25372 at commit 8dd1feb.

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

@dongjoon-hyun
Copy link
Member

Retest this please.

} catch {
case _: CatalogNotFoundException =>
logWarning("Session catalog is not defined")
None
Copy link
Member

@dongjoon-hyun dongjoon-hyun Aug 11, 2019

Choose a reason for hiding this comment

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

@jzhuge Could you print out more information of CatalogNotFoundException without the full stack trace?
Without that, the previous error message will be better for users because it gives the exact reason Cannot find catalog plugin class for catalog 'session': xxx.

$ bin/spark-shell --conf spark.sql.catalog.session=xxx
scala> spark.sessionState.analyzer.sessionCatalog
19/08/10 18:17:29 ERROR HiveSessionStateBuilder$$anon$1: Cannot load v2 session catalog
org.apache.spark.SparkException: Cannot find catalog plugin class for catalog 'session': xxx

Copy link
Member

Choose a reason for hiding this comment

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

BTW, @jzhuge . Is this all places to give a warning instead of the stacks traces?
With this PR, I still see the previous behavior.

Copy link
Member Author

@jzhuge jzhuge Aug 12, 2019

Choose a reason for hiding this comment

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

@dongjoon-hyun Thanks for the review. Your command line is not the case I tried to fix in the PR. In your case, the stack trace is helpful.

It seems that the current master has session catalog defined by default, so here is the command line to reproduce my case:

$ bin/spark-shell --master 'local[*]' --conf spark.sql.catalog.session=
...
Spark context available as 'sc' (master = local[*], app id = local-1565588237201).
Spark session available as 'spark'.
...
scala> spark.sessionState.analyzer.sessionCatalog
...
2019-08-11 22:37:24,216 ERROR [main] hive.HiveSessionStateBuilder$$anon$1 (Logging.scala:logError(94)) - Cannot load v2 session catalog
org.apache.spark.SparkException: Cannot find catalog plugin class for catalog 'session':
	at org.apache.spark.sql.catalog.v2.Catalogs.load(Catalogs.java:81)
...
res0: Option[org.apache.spark.sql.catalog.v2.CatalogPlugin] = None

Here the stack trace does not add more information. And I am concerned that if any rule uses session catalog, we will see this long stack trace again and again.

Copy link
Member

Choose a reason for hiding this comment

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

Could you fix both cases in this PR? I believe typo cases will be more frequent than the empty configuration cases.

@SparkQA
Copy link

SparkQA commented Aug 11, 2019

Test build #108926 has finished for PR 25372 at commit 8dd1feb.

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

@jzhuge
Copy link
Member Author

jzhuge commented Aug 19, 2019

Handle typo in plugin class name for both v2 session catalog and default catalog.

Manual test cases

  • bin/spark-shell --conf spark.sql.catalog.session=noclass
  • bin/spark-shell --conf spark.sql.default.catalog=def
  • bin/spark-shell --conf spark.sql.default.catalog=def --conf spark.sql.catalog.def=noclass

@SparkQA
Copy link

SparkQA commented Aug 19, 2019

Test build #109317 has finished for PR 25372 at commit d55a430.

  • This patch fails Java style tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class CatalogClassNotFoundException(message: String, cause: Throwable)

@jzhuge jzhuge changed the title [SPARK-28640][SQL] Only give warning when session catalog is not defined [SPARK-28640][SQL] Do not show stack trace when default or session catalog is misconfigured Aug 19, 2019
@SparkQA
Copy link

SparkQA commented Aug 19, 2019

Test build #109364 has finished for PR 25372 at commit f814c52.

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

@dongjoon-hyun
Copy link
Member

Sorry for being late. Could you resolve the conflicts, @jzhuge ?

…talog is misconfigured

LookupCatalog.sessionCatalog logs an error message and the exception stack upon any nonfatal exception.
When session catalog is just misconfigured, this may alarm the user unnecessarily.
It should be enough to give a warning and return None.

- bin/spark-shell --conf spark.sql.catalog.session=noclass
- bin/spark-shell --conf spark.sql.default.catalog=def
- bin/spark-shell --conf spark.sql.default.catalog=def --conf spark.sql.catalog.def=noclass
@SparkQA
Copy link

SparkQA commented Aug 25, 2019

Test build #109692 has finished for PR 25372 at commit 1228281.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class CatalogClassNotFoundException(message: String, cause: Throwable)

@jzhuge
Copy link
Member Author

jzhuge commented Aug 25, 2019

Is this test failure related to the patch?

[error] running /home/jenkins/workspace/SparkPullRequestBuilder@2/build/sbt -Phadoop-2.7 -Phive-thriftserver -Phive -Dtest.exclude.tags=org.apache.spark.tags.ExtendedHiveTest,org.apache.spark.tags.ExtendedYarnTest hive-thriftserver/test avro/test mllib/test hive/test repl/test catalyst/test sql/test sql-kafka-0-10/test examples/test ; process was terminated by signal 9

@kiszk
Copy link
Member

kiszk commented Sep 3, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Sep 3, 2019

Test build #110052 has finished for PR 25372 at commit 1228281.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class CatalogClassNotFoundException(message: String, cause: Throwable)

@github-actions
Copy link

We're closing this PR because it hasn't been updated in a while.
This isn't a judgement on the merit of the PR in any way. It's just
a way of keeping the PR queue manageable.

If you'd like to revive this PR, please reopen it!

@github-actions github-actions bot added the Stale label Dec 26, 2019
@github-actions github-actions bot closed this Dec 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants