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

HDDS-2659. KeyValueHandler#handleCreateContainer should log the exception on container creation failure #296

Merged
merged 2 commits into from Dec 12, 2019

Conversation

cxorm
Copy link
Member

@cxorm cxorm commented Dec 4, 2019

What changes were proposed in this pull request?

Let the StorageContainerException cause of KeyValueHandler#handleCreateContaine be logged.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-2659

How was this patch tested?

Just build and compile.

@@ -146,6 +146,7 @@ public static ContainerCommandResponseProto logAndReturnError(
log.info("Operation: {} , Trace ID: {} , Message: {} , Result: {}",
request.getCmdType().name(), request.getTraceID(),
ex.getMessage(), ex.getResult().getValueDescriptor().getName());
log.error("StorageContainerException Occurred!", ex);
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have a log.info just above it with exception message and the result. If more information about the exception is needed, can we consolidate it into the previous message.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @xiaoyuyao for the comment.
Updated.

@cxorm cxorm requested a review from xiaoyuyao December 5, 2019 10:23
@anuengineer
Copy link
Contributor

Not sure why the checks failed. This patch should not cause it. I have requested a re-run of the tests.

Copy link
Contributor

@bharatviswa504 bharatviswa504 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

Copy link
Contributor

@mukul1987 mukul1987 left a comment

Choose a reason for hiding this comment

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

+1, the patch looks good to me.

@mukul1987 mukul1987 merged commit 0b1ea25 into apache:master Dec 12, 2019
@cxorm
Copy link
Member Author

cxorm commented Dec 12, 2019

Thanks @xiaoyuyao, @anuengineer and @bharatviswa504 for the reviews.
And thanks @mukul1987 for the merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants