-
Notifications
You must be signed in to change notification settings - Fork 994
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
PHOENIX-6185 : Propagate info available in SQLExceptionInfo to SQLTimeoutException #919
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
💔 -1 overall
This message was automatically generated. |
It's strange that build data is not available on ci-hadoop. Perhaps some clearn up took place? |
|
💔 -1 overall
This message was automatically generated. |
@gjacoby126 checkstyle is fixed, should be visible in next build. This applies directly to 4.x. Would you like me to create 4.x PR? |
phoenix-core/src/main/java/org/apache/phoenix/exception/SQLExceptionCode.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just 1 minor comment. other than ltgm.
(info.getMessage() != null ? info.getMessage() : "") | ||
+ (info.getRootCause() != null ? " , rootCause: " | ||
+ info.getRootCause() : ""); | ||
return new SQLTimeoutException(reason, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry missed this in my previous review.
If there is root cause exception present, you should use this constructor to create SQLtimeoutException
public SQLTimeoutException(String reason,
String SQLState,
int vendorCode,
Throwable cause)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
assertEquals(OPERATION_TIMED_OUT.getErrorCode(), | ||
e.getErrorCode()); | ||
assertTrue(e.getMessage().contains("Query couldn't be " + | ||
"completed in the allotted time: 5000 ms")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check that e.getCause() != null ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it is null in this case because for the test, rootCause is not provided here (BaseResultIterators):
throw new SQLExceptionInfo.Builder(OPERATION_TIMED_OUT).setMessage(
". Query couldn't be completed in the allotted time: "
+ queryTimeOut + " ms").build().buildException();
However, I tested by manually adding rootCause and receiving on test side. Hence, it is working fine but since relevant source code does not add root cause, e.getCause() != null
would not be true here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@virajjasani Can we just write a unit test creating OPERATION_TIMED_OUT exception with custom message and custom fake exception. Ideally we wouldn't even need integration test but the test that you wrote is very nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Let's keep both unit and IT.
Thanks
💔 -1 overall
This message was automatically generated. |
phoenix-core/src/test/java/org/apache/phoenix/util/SQLExceptionCodeTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 ltgm pending jenkins run. Thank you @virajjasani for fixing nitty comments. :)
💔 -1 overall
This message was automatically generated. |
…eoutException (#919) PHOENIX-6185 : Propagate info available in SQLExceptionInfo to OPERATION_TIMED_OUT
No description provided.