Skip to content
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.

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

[ISSUE #2138] add the global exception handler, remove the duplicate try catch #2223

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

brotherlu-xcq
Copy link
Collaborator

Describe what this PR does / why we need it

Fixes #2138
add the global exception handler, remove the duplicate try catch

Does this pull request fix one issue?

Fixes #2138

Describe how you did it

add GlobalExceptionHandler to catch the exception and format the error return.

Describe how to verify it

See the screenshot after change.

image

Special notes for reviews

@sczyh30 sczyh30 added area/dashboard Issues or PRs about Sentinel Dashboard kind/enhancement Category issues or prs related to enhancement. labels Jun 2, 2021
@sczyh30 sczyh30 requested a review from cdfive June 2, 2021 11:11
@brotherlu-xcq
Copy link
Collaborator Author

@cdfive I have solved the problems. could you help to review it again when you are free. tks.

@cdfive
Copy link
Collaborator

cdfive commented Jun 6, 2021

@brotherlu-xcq
Nice work! I've no further idea. Unified global exception handling makes business code more elegant and concise.

Map<String, String[]> params = webRequest.getParameterMap();
String contextPath = getRequestPath(webRequest);
logger.error("[HandleExecutionException] Request failed, context path: {}, request params: {}, error: ",
contextPath, buildParamMessage(params), e);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe e.getCause() is better here (for ExecutionException)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @sczyh30 , I tried this. print the e will also print the cause exception. I think maybe we can keep it as now.

Copy link
Member

@sczyh30 sczyh30 left a comment

Choose a reason for hiding this comment

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

LGTM

CST11021 pushed a commit to CST11021/Sentinel that referenced this pull request Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dashboard Issues or PRs about Sentinel Dashboard kind/enhancement Category issues or prs related to enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

console error info not friendly
3 participants