HDDS-1886. Use ArrayList#clear to address audit failure scenario #1205
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/label ozone |
adoroszlai
reviewed
Aug 1, 2019
| @@ -153,7 +153,7 @@ private void verifyLog(String expected) throws IOException { | |||
| assertTrue(lines.size() != 0); | |||
| assertTrue(expected.equalsIgnoreCase(lines.get(0))); | |||
| //empty the file | |||
| lines.remove(0); | |||
| lines.clear(); | |||
There was a problem hiding this comment.
@dineshchitlangia good idea to avoid unnecessary assumptions.
Here are some further suggestions:
- Wrap assertions and cleanup in
tryandfinally, respectively. Otherwise a failed assertion would cause all further independent test cases to also fail due to leftover content in the file. (This one is not specific to multi-line cases.) - Make
verifyLogacceptString...and check each expected line. Also, retry reading the file until it has enough lines instead of being non-empty. - I think using
assertEqualswould make it easier to spot differences between expected and actual values. (I'm not sure case-ignorance is really important. Currently the test passes with strict case check, too.)
There was a problem hiding this comment.
@adoroszlai Thanks for review & suggestions.
- Wrap assertions and cleanup in
tryandfinally, respectively. Otherwise a failed assertion would cause all further independent test cases to also fail due to leftover content in the file. (This one is not specific to multi-line cases.)
Yes, that was the original approach, however, the review at the time preferred to throw the Exception instead of catching it. This is because we won't accept a few failures, the criteria required is all tests must pass.
- Make
verifyLogacceptString...and check each expected line. Also, retry reading the file until it has enough lines instead of being non-empty.
In this test for the base framework of audit logging, we invoke verifyLog each time a log worthy event has occurred. So at any time, the recent event is the only event in the audit log. Hence we verify only one line at a time as they are invoked by different tests.
- I think using
assertEqualswould make it easier to spot differences between expected and actual values. (I'm not sure case-ignorance is really important. Currently the test passes with strict case check, too.)
Again, since the test was for base audit logging framework, strict checking was required in the original.
That said, the goal of current jira is only to move away from using ArrayList#remove.
I have filed HDDS-1889 to address the multi-line log scenario that will cover your suggestions.
There was a problem hiding this comment.
@adoroszlai Could please you let me know if there are any other concerns based on my response above and follow up jira? Thanks!
There was a problem hiding this comment.
@dineshchitlangia Thanks for clarifying the scope and filing the follow-up Jira. I don't have any other concerns.
There was a problem hiding this comment.
@adoroszlai Thanks for the review. I will commit this now.
|
Checkstyle & Integrations failures are unrelated to the patch |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.