Skip to content

[SPARK-36100][CORE] Grouping exception in core/status#33555

Closed
dgd-contributor wants to merge 7 commits intoapache:masterfrom
dgd-contributor:SPARK-36100
Closed

[SPARK-36100][CORE] Grouping exception in core/status#33555
dgd-contributor wants to merge 7 commits intoapache:masterfrom
dgd-contributor:SPARK-36100

Conversation

@dgd-contributor
Copy link

What changes were proposed in this pull request?

This PR group exception messages in core/src/main/scala/org/apache/spark/status

Why are the changes needed?

It will largely help with standardization of error messages and its maintenance.

Does this PR introduce any user-facing change?

No. Error messages remain unchanged.

How was this patch tested?

No new tests - pass all original tests to make sure it doesn't break any existing behavior.

@github-actions github-actions bot added the CORE label Jul 28, 2021
@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@mridulm
Copy link
Contributor

mridulm commented Jul 28, 2021

I have not followed on this work, but looking at this pr a general comment, please use as specific a subclass as possible without loosing generalizability while defining interfaces.
For example, use Exception instead of Throwable - or a relevant subclass, like SparkException, as relevant.

@cloud-fan
Copy link
Contributor

please use as specific a subclass as possible without loosing generalizability while defining interfaces.

Yea I think this is the policy. The recent work is just to centralize all the errors into one or a few files. The error message and exception class should not be changed for now.

Copy link
Contributor

@allisonwang-db allisonwang-db left a comment

Choose a reason for hiding this comment

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

Looks good!

new ForbiddenException(s"""user "$user" is not authorized""")
}

def notFoundAppKeyError(appKey: String): Throwable = {
Copy link
Contributor

Choose a reason for hiding this comment

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

appNotFoundError

new NotFoundException(s"no such app: $appKey")
}

def notFoundJobIdError(jobId: Int): Throwable = {
Copy link
Contributor

Choose a reason for hiding this comment

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

unknownJobError

new NotFoundException("No thread dump is available.")
}

def notFoundHttpRequestError(uri: String): Throwable = {
Copy link
Contributor

Choose a reason for hiding this comment

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

uriNotFoundError

new NotFoundException(s"unknown app: $appId")
}

def notFoundAppWithAttemptError(appId: String, attemptId: String): Throwable = {
Copy link
Contributor

Choose a reason for hiding this comment

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

unknownAppWithAttemptError

new NotFoundException(s"unknown attempt for stage $stageId. Found attempts: [$msg]")
}

def noTasksReportMetricsError(stageId: Int, stageAttemptId: Int): Throwable = {
Copy link
Contributor

Choose a reason for hiding this comment

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

noTaskReportedMetricsError

new NotFoundException(s"No tasks reported metrics for $stageId / $stageAttemptId yet.")
}

def badParameterErrors(param: String, exp: String, actual: String): Throwable = {
Copy link
Contributor

Choose a reason for hiding this comment

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

badParameterError

@dgd-contributor
Copy link
Author

dgd-contributor commented Oct 6, 2021

Thanks for your reviews. As we and @HyukjinKwon discussed contributing by sharing accounts, we will close this PR and recreate another by individual account.

@dgd-contributor
Copy link
Author

Please see PR #34191

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.

6 participants