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
ACCUMULO-4546 Create default log message for IllegalTableTransitionException #327
Conversation
Updated the IllegalTableTransitionException class within TableManager.java to print a default log message when exception is thrown. User also has option of supplying a custom message as well. Created a unit test as well.
…ception Updated the IllegalTableTransitionException class within TableManager.java to have a default log message when exception is thrown. User also has option of supplying a custom message. Created unit test. Moved location of an import in AccumuloTest.java to make mvn happy.
…xception Updated the IllegalTableTransitionException class within TableManager.java to have a default log message when exception is thrown. User also has option of supplying a custom message. Created unit test. Moved location of an import in AccumuloTest.java to make mvn happy.
…ception ACCUMULO-4546 Updated the IllegalTableTransitionException class within TableManager.java to print a default log message when exception is thrown. User also has option of supplying a custom message. Created unit test. Moved import in AccumuloTest.java to make mvn happy.
…ception ACCUMULO-4546 Updated the IllegalTableTransitionException class within TableManager.java to print a default log message when exception is thrown. User also has option of supplying a custom message. Created unit test. Moved import in AccumuloTest.java to make mvn happy.
…xception Updated the IllegalTableTransitionException class within TableManager.java to have a default log message when exception is thrown. User also has option of supplying a custom message. Created unit test. Moved location of an import in AccumuloTest.java to make mvn happy.
Can one of the admins verify this patch? |
@Test | ||
public void testIllegalTableTransitionExceptionEmptyMessage() { | ||
try { | ||
throw new TableManager.IllegalTableTransitionException(oldState, newState, ""); |
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.
In addition to ""
, could also try 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.
I went back and forth between null and the empty string. Both will work. I can go with either if you have a preference.
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 saw both were supported in the code. I was thinking could have test for both.
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.
Added additional unit test.
this.oldState = oldState; | ||
this.newState = newState; | ||
|
||
String defaultMessage = "Error transitioning from " + oldState + " state to " + newState + " state"; |
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.
Could this be moved inside the else block?
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.
Yes...done.
…ception Moved the defaultMessage declaration inside the if/else block.
this.oldState = oldState; | ||
this.newState = newState; | ||
|
||
if (message != null && !message.isEmpty()) |
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.
Could use StringUtils.isNotEmpty() from apache commons. Or even static import it so it just reads if(isNotEmpty(message))
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.
Thanks, @milleruntime . I like that better.
…ception Added unit test for null message. Also, updated message check to use StringUtils.isNotEmpty()
ACCUMULO-4546 Create default log message for IllegalTableTransitionException
Updated the IllegalTableTransitionException class within TableManager.java to print
a default log message when exception is thrown. User also has option of supplying a
custom message. Created unit test.
Moved import in AccumuloTest.java to make mvn happy.