Skip to content

Comments

[SPARK-43428] Move some class utils to common/utils#41109

Closed
amaliujia wants to merge 2 commits intoapache:masterfrom
amaliujia:move_some_utils
Closed

[SPARK-43428] Move some class utils to common/utils#41109
amaliujia wants to merge 2 commits intoapache:masterfrom
amaliujia:move_some_utils

Conversation

@amaliujia
Copy link
Contributor

What changes were proposed in this pull request?

This PR moves some commonly used class loader/reflection utils to common/utils.

Why are the changes needed?

Reduce the required dependency on Spark core.

Does this PR introduce any user-facing change?

NO

How was this patch tested?

Existing UT

@amaliujia
Copy link
Contributor Author

@hvanhovell @cloud-fan

@github-actions github-actions bot added the CORE label May 9, 2023
*/
package org.apache.spark.util

object SparkClassUtils {
Copy link
Member

Choose a reason for hiding this comment

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

This object name SparkClassUtils doesn't match with the file name, ClassUtils.scala.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yeah updated.

Copy link
Contributor

@hvanhovell hvanhovell left a comment

Choose a reason for hiding this comment

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

LGTM

* Get the ClassLoader which loaded Spark.
*/
def getSparkClassLoader: ClassLoader = getClass.getClassLoader
def getSparkClassLoader: ClassLoader = SparkClassUtils.getSparkClassLoader
Copy link
Contributor

Choose a reason for hiding this comment

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

You implement SparkClassUtils as a trait, and make Utils implement that trait. That will save you from writing a lot of code like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good idea.

@hvanhovell
Copy link
Contributor

Merging.

LuciferYang pushed a commit to LuciferYang/spark that referenced this pull request May 10, 2023
### What changes were proposed in this pull request?

This PR moves some commonly used class loader/reflection utils to common/utils.

### Why are the changes needed?

Reduce the required dependency on Spark core.

### Does this PR introduce _any_ user-facing change?

NO
### How was this patch tested?

Existing UT

Closes apache#41109 from amaliujia/move_some_utils.

Authored-by: Rui Wang <rui.wang@databricks.com>
Signed-off-by: Herman van Hovell <herman@databricks.com>
@LuciferYang
Copy link
Contributor

LuciferYang commented May 11, 2023

A late question: Should error/error-classes.json and SparkThrowableHelper be moved together to the common-utils module?

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.

4 participants