-
Notifications
You must be signed in to change notification settings - Fork 153
[TRAFODION-1988] Better java exception handling in the java/JNI layer #714
Conversation
The stack trace of the exception displayed from JNI side now contains the cause of the exception displayed in a nested way.
Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1154/ |
Test Failed. https://jenkins.esgyn.com/job/Check-PR-master/1154/ |
Fix for the check-PR failure in seabase test suite
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1158/ |
Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/1158/ |
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.
Looks good to me.
return true; | ||
} | ||
|
||
void JavaObjectInterfaceTM::appendExceptionMessages(JNIEnv *jenv, jthrowable a_exception, std::string *error_msg) |
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.
If we don't expect error_msg to be null, it might be better to code this as a reference parameter rather than a pointer.
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.
@dave fixed as per your comments
Fixes as per the review comments TM exception logging was at times dumping TM core because log4cxx infrastructure was not initialized early in the process startup. Initialized it as early as it can be done.
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1167/ |
Test Failed. https://jenkins.esgyn.com/job/Check-PR-master/1167/ |
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 Looks good to me.
Changes to fix the regression failure
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/1172/ |
Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/1172/ |
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.
I'm not an expert on the logging code, but the changes seem fine to me.
+1. Executor portion looks very nice. I am curious to see what kind of output the getCause method produces. If you have a sample handy, please add it to the JIRA. |
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.
Looks good.
Changes look good. This change will also help during startup if TM encounters exceptions connecting to Hbase. |
Looks good to me. |
The stack trace of the exception displayed from JNI side now contains the
cause of the exception displayed in a nested way.