Skip to content

Conversation

@llemec
Copy link
Contributor

@llemec llemec commented Aug 5, 2020

What changes were proposed in this pull request?

Added multi line support for Audit messages in TestOzoneAuditLogger

What is the link to the Apache JIRA

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

How was this patch tested?

added messageIncludesMultilineException in TestOzoneAuditLogger

@llemec llemec changed the title Hdds 1889 HDDS-1889 Add support for verifying multiline log entry Aug 6, 2020
@fapifta
Copy link
Contributor

fapifta commented Aug 6, 2020

Hi @llemec,

first of all thank you for the contribution, acceptance test failures are related to recon, after they are fixed, it would be necessary to have a clean build before a commiter can merge the changeset, please ensure we have one.

I would argue that we should check the order of the messages as well, which would not be that hard to implement neither with your approach nor with the approach I am suggesting inline.

I have some simplification suggestions in the inline comments, please consider them, and if you like the idea, please include them in the PR.

@fapifta
Copy link
Contributor

fapifta commented Aug 10, 2020

Hi @llemec,

thank you for working further on this PR, and addressing my suggestions.
It seems that a few commits somehow mixed into the branch from where the PR is initiated...
Not sure what went wrong, but this one unfortunately needs to be fixed... worst case cancel this PR, and add a new one from a clean branch containing your changes only ;)

@vivekratnavel might be able to help with the coverage problem in the checks, I am not sure but this one seems to be a non-generic issue

Besides that,

  • please address the checkstyle issues remained in the code
  • the contains method is not needed anymore, can we please remove that from the test class?

@llemec
Copy link
Contributor Author

llemec commented Aug 10, 2020

Hello @fapifta,

Thank you for your review and suggestions. I am proceeding to cancel this PR and re-submit it via a clean branch, together with the checkstyle fixes and the removal of unnecessary methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants