Skip to content

[SPARK-39958][SQL] Add warning log when unable to load custom metric object #37386

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.

Sign up for GitHub

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

Jump to bottom
Closed
viirya wants to merge 2 commits into from
Closed

[SPARK-39958][SQL] Add warning log when unable to load custom metric object #37386

viirya wants to merge 2 commits into from

Conversation

@viirya
Copy link
Member

@viirya viirya commented Aug 3, 2022

What changes were proposed in this pull request?

Currently when SQLAppStatusListener cannot load custom metric object, it silently ignores the error. We better provide a warning log for users.

Why are the changes needed?

Improving user experience.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing tests

@viirya viirya mentioned this pull request Aug 3, 2022
Closed
@viirya
Copy link
Member Author

viirya commented Aug 3, 2022

cc @dongjoon-hyun

@pan3793
Copy link
Member

pan3793 commented Aug 3, 2022

Previous discussion: #31451 (comment)

I think we don't need to construct the instance using reflection, how about removing constructor checks?

@cloud-fan
Copy link
Contributor

cloud-fan commented Aug 3, 2022

I think we don't need to construct the instance using reflection

Then how can we construct the instance?

@viirya
Copy link
Member Author

viirya commented Aug 3, 2022

Just about to ask same question.

@pan3793
Copy link
Member

pan3793 commented Aug 3, 2022

Oh, I see. SQLMetrics.createV2CustomMetric only store the class name instead of the instance itself, then constructing the instance by reflection is necessary.

Previous I misunderstand that instances constructed by Scan#supportedCustomMetrics will be passed and reused.

Could you please enhance the comment of CustomMetric to mention the constructor requirements as well?

@viirya
Copy link
Member Author

viirya commented Aug 3, 2022

Could you please enhance the comment of CustomMetric to mention the constructor requirements as well?

Okay.

dongjoon-hyun
dongjoon-hyun reviewed Aug 3, 2022
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/ui/SQLAppStatusListener.scala Outdated
case NonFatal(e) =>
logWarning(s"Unable to load custom metric object for class `$className`. " +
"Please make sure that the custom metric class is in the classpath and " +
"it has 0-arg constructor", e)
Copy link
Member

@dongjoon-hyun dongjoon-hyun Aug 3, 2022

Choose a reason for hiding this comment

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

nit. . at the end of the sentence?

Copy link
Member Author

@viirya viirya Aug 3, 2022

Choose a reason for hiding this comment

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

Added. Thanks.

dongjoon-hyun
dongjoon-hyun approved these changes Aug 3, 2022
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM.

pan3793
pan3793 reviewed Aug 3, 2022
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/ui/SQLAppStatusListener.scala
case NonFatal(e) =>
logWarning(s"Unable to load custom metric object for class `$className`. " +
"Please make sure that the custom metric class is in the classpath and " +
"it has 0-arg constructor.", e)
Copy link
Member

@pan3793 pan3793 Aug 3, 2022

Choose a reason for hiding this comment

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

Utils#loadExtensions also allows "single-argument constructor that accepts SparkConf", do we need to mention that?

Copy link
Member Author

@viirya viirya Aug 3, 2022

Choose a reason for hiding this comment

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

I don't think we should. Actually we don't need the implementing class to implement it.

pan3793
pan3793 approved these changes Aug 3, 2022
View reviewed changes
Copy link
Member

@pan3793 pan3793 left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@viirya
Copy link
Member Author

viirya commented Aug 3, 2022

The known and unrelated sparkr failure:

Status: 1 WARNING, 2 NOTEs
See
  ‘/__w/spark-1/spark-1/R/SparkR.Rcheck/00check.log’
for details.


+ popd
Had CRAN check errors; see logs.
[error] running /__w/spark-1/spark-1/R/run-tests.sh ; received return code 255

Unrelated Python linter:

Traceback (most recent call last):
  File "/usr/lib/python3.9/runpy.py", line 197, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/lib/python3.9/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/usr/local/lib/python3.9/dist-packages/black/__main__.py", line 3, in <module>
    patched_main()
  File "/usr/local/lib/python3.9/dist-packages/black/__init__.py", line 1372, in patched_main
    patch_click()
  File "/usr/local/lib/python3.9/dist-packages/black/__init__.py", line 1358, in patch_click
    from click import _unicodefun
ImportError: cannot import name '_unicodefun' from 'click' (/usr/local/lib/python3.9/dist-packages/click/__init__.py)
Please run 'dev/reformat-python' script.

@viirya viirya closed this in 1e224e6 Aug 3, 2022
@viirya
Copy link
Member Author

viirya commented Aug 3, 2022

Merged, thanks!

@viirya viirya deleted the minor_log branch December 27, 2023 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

+1 more reviewer

@pan3793 pan3793 pan3793 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

SQL WEB UI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@viirya @pan3793 @cloud-fan @dongjoon-hyun