You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/08/05 16:19:17 UTC
[GitHub] [hadoop-ozone] llemec opened a new pull request #1293: Hdds 1889
llemec opened a new pull request #1293:
URL: https://github.com/apache/hadoop-ozone/pull/1293
## 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] llemec commented on a change in pull request #1293: HDDS-1889 Add support for verifying multiline log entry
Posted by GitBox <gi...@apache.org>.
llemec commented on a change in pull request #1293:
URL: https://github.com/apache/hadoop-ozone/pull/1293#discussion_r467139318
##########
File path: hadoop-hdds/common/src/test/java/org/apache/hadoop/ozone/audit/TestOzoneAuditLogger.java
##########
@@ -143,7 +143,31 @@ public void notLogReadEvents() throws IOException {
verifyNoLog();
}
- private void verifyLog(String expected) throws IOException {
+ /**
+ * Test to verify if multiline entries can be checked.
+ */
+
+ @Test
+ public void messageIncludesMultilineException() throws IOException {
+ String exceptionMessage = "Dummuy exception message";
Review comment:
fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] fapifta commented on pull request #1293: HDDS-1889 Add support for verifying multiline log entry
Posted by GitBox <gi...@apache.org>.
fapifta commented on pull request #1293:
URL: https://github.com/apache/hadoop-ozone/pull/1293#issuecomment-671310511
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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] fapifta commented on pull request #1293: HDDS-1889 Add support for verifying multiline log entry
Posted by GitBox <gi...@apache.org>.
fapifta commented on pull request #1293:
URL: https://github.com/apache/hadoop-ozone/pull/1293#issuecomment-669902175
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] fapifta commented on a change in pull request #1293: HDDS-1889 Add support for verifying multiline log entry
Posted by GitBox <gi...@apache.org>.
fapifta commented on a change in pull request #1293:
URL: https://github.com/apache/hadoop-ozone/pull/1293#discussion_r466381992
##########
File path: hadoop-hdds/common/src/test/java/org/apache/hadoop/ozone/audit/TestOzoneAuditLogger.java
##########
@@ -158,20 +184,34 @@ private void verifyLog(String expected) throws IOException {
}
i++;
}
-
- // When log entry is expected, the log file will contain one line and
- // that must be equal to the expected string
- assertTrue(lines.size() != 0);
- assertTrue(expected.equalsIgnoreCase(lines.get(0)));
+ //check if every expected string can be found in the log entry
+ for(String expected : expectedStrings) {
+ assertTrue(contains(lines, expected));
Review comment:
instead of writing a contains method we can use the following combination of assertThat and matchers to gain the same functionality:
```java
assertThat(lines, hasItem(containsString(expected)));
```
If we change to check that lines starts with the lines specified in expectedStrings in order, then an other approach like this solves the problem without the for loop:
```java
assertThat(
lines.subList(0,expectedStrings.length),
containsInOrder(expectedStrings)
);
```
where containsInOrder is defined as:
```java
private Matcher<Iterable<? extends String>> containsInOrder(
String[] expectedStrings) {
return IsIterableContainingInOrder.contains(
Arrays.stream(expectedStrings)
.map(str -> containsString(str))
.collect(Collectors.toList())
);
}
```
In order for this to work it is necessary to add org-harmcrest:harmcrest-all as a test scope dependency to the hdds-common module, and change the messageIncludesMultilineException test, to call verifyLog as:
```java
verifyLog(
"ERROR | OMAudit | user=john | ip=192.168.0.1 | op=CREATE_VOLUME " +
"{key1=value1, key2=value2} | ret=FAILURE",,
"TestException: " + exceptionMsg,
"org.apache.hadoop.ozone.audit."
+ "TestOzoneAuditLogger."
+ "messageIncludesMultilineException"
);
```
assuming that the message text given to the TestException is extracted to a local variable called exceptionMsg ;)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] fapifta commented on a change in pull request #1293: HDDS-1889 Add support for verifying multiline log entry
Posted by GitBox <gi...@apache.org>.
fapifta commented on a change in pull request #1293:
URL: https://github.com/apache/hadoop-ozone/pull/1293#discussion_r466381992
##########
File path: hadoop-hdds/common/src/test/java/org/apache/hadoop/ozone/audit/TestOzoneAuditLogger.java
##########
@@ -158,20 +184,34 @@ private void verifyLog(String expected) throws IOException {
}
i++;
}
-
- // When log entry is expected, the log file will contain one line and
- // that must be equal to the expected string
- assertTrue(lines.size() != 0);
- assertTrue(expected.equalsIgnoreCase(lines.get(0)));
+ //check if every expected string can be found in the log entry
+ for(String expected : expectedStrings) {
+ assertTrue(contains(lines, expected));
Review comment:
instead of writing a contains method we can use the following combination of assertThat and matchers to gain the same functionality:
```java
assertThat(lines, hasItem(containsString(expected)));
```
If we change to check that lines starts with the lines specified in expectedStrings in order, then an other approach like this solves the problem:
```java
assertThat(
lines.subList(0,expectedStrings.length),
containsInOrder(expectedStrings)
);
```
where containsInOrder is defined as:
```java
private Matcher<Iterable<? extends String>> containsInOrder(
String[] expectedStrings) {
return IsIterableContainingInOrder.contains(
Arrays.stream(expectedStrings)
.map(str -> containsString(str))
.collect(Collectors.toList())
);
}
```
In order for this to work it is necessary to add org-harmcrest:harmcrest-all as a test scope dependency to the hdds-common module, and change the messageIncludesMultilineException test, to call verifyLog as:
```java
verifyLog(
"ERROR | OMAudit | user=john | ip=192.168.0.1 | op=CREATE_VOLUME " +
"{key1=value1, key2=value2} | ret=FAILURE",,
"TestException: " + exceptionMsg,
"org.apache.hadoop.ozone.audit."
+ "TestOzoneAuditLogger."
+ "messageIncludesMultilineException"
);
```
assuming that the message text given to the TestException is extracted to a local variable called exceptionMsg ;)
##########
File path: hadoop-hdds/common/src/test/java/org/apache/hadoop/ozone/audit/TestOzoneAuditLogger.java
##########
@@ -143,7 +143,33 @@ public void notLogReadEvents() throws IOException {
verifyNoLog();
}
- private void verifyLog(String expected) throws IOException {
+ /**
+ * Test to verify if multiline entries can be checked.
+ */
+
+ @Test
+ public void messageIncludesMultilineException() throws IOException {
+ try{
+ throw new TestException("Dummy exception");
Review comment:
One don't need to throw the exception in order to pass it to the withException method and log it correctly, so the test could be simpler by just specifying a new TestExecption("Dummy") to the withException method on the builder.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] llemec closed pull request #1293: HDDS-1889 Add support for verifying multiline log entry
Posted by GitBox <gi...@apache.org>.
llemec closed pull request #1293:
URL: https://github.com/apache/hadoop-ozone/pull/1293
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] llemec commented on a change in pull request #1293: HDDS-1889 Add support for verifying multiline log entry
Posted by GitBox <gi...@apache.org>.
llemec commented on a change in pull request #1293:
URL: https://github.com/apache/hadoop-ozone/pull/1293#discussion_r466527257
##########
File path: hadoop-hdds/common/src/test/java/org/apache/hadoop/ozone/audit/TestOzoneAuditLogger.java
##########
@@ -143,7 +143,33 @@ public void notLogReadEvents() throws IOException {
verifyNoLog();
}
- private void verifyLog(String expected) throws IOException {
+ /**
+ * Test to verify if multiline entries can be checked.
+ */
+
+ @Test
+ public void messageIncludesMultilineException() throws IOException {
+ try{
+ throw new TestException("Dummy exception");
Review comment:
agreed and fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] llemec commented on pull request #1293: HDDS-1889 Add support for verifying multiline log entry
Posted by GitBox <gi...@apache.org>.
llemec commented on pull request #1293:
URL: https://github.com/apache/hadoop-ozone/pull/1293#issuecomment-671314072
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] fapifta commented on a change in pull request #1293: HDDS-1889 Add support for verifying multiline log entry
Posted by GitBox <gi...@apache.org>.
fapifta commented on a change in pull request #1293:
URL: https://github.com/apache/hadoop-ozone/pull/1293#discussion_r466381992
##########
File path: hadoop-hdds/common/src/test/java/org/apache/hadoop/ozone/audit/TestOzoneAuditLogger.java
##########
@@ -158,20 +184,34 @@ private void verifyLog(String expected) throws IOException {
}
i++;
}
-
- // When log entry is expected, the log file will contain one line and
- // that must be equal to the expected string
- assertTrue(lines.size() != 0);
- assertTrue(expected.equalsIgnoreCase(lines.get(0)));
+ //check if every expected string can be found in the log entry
+ for(String expected : expectedStrings) {
+ assertTrue(contains(lines, expected));
Review comment:
instead of writing a contains method we can use the following combination of assertThat and matchers to gain the same functionality:
```java
assertThat(lines, hasItem(containsString(expected)));
```
If we change to check that lines starts with the lines specified in expectedStrings in order, then an other approach like this solves the problem without the for loop:
```java
assertThat(
lines.subList(0,expectedStrings.length),
containsInOrder(expectedStrings)
);
```
where containsInOrder is defined as:
```java
private Matcher<Iterable<? extends String>> containsInOrder(
String[] expectedStrings) {
return IsIterableContainingInOrder.contains(
Arrays.stream(expectedStrings)
.map(str -> containsString(str))
.collect(Collectors.toList())
);
}
```
In order for this to work it is necessary to add org-harmcrest:harmcrest-all as a test scope dependency to the hdds-common module, and change the messageIncludesMultilineException test, to call verifyLog as:
```java
verifyLog(
"ERROR | OMAudit | user=john | ip=192.168.0.1 | op=CREATE_VOLUME " +
"{key1=value1, key2=value2} | ret=FAILURE",,
"TestException: " + exceptionMsg,
"org.apache.hadoop.ozone.audit."
+ "TestOzoneAuditLogger."
+ "messageIncludesMultilineException"
);
```
assuming that the message text given to the TestException is extracted to a local variable called exceptionMsg, though it may even be better to use TestException.class.getName() +": " + exceptionMsg to be refactorproof :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] llemec commented on a change in pull request #1293: HDDS-1889 Add support for verifying multiline log entry
Posted by GitBox <gi...@apache.org>.
llemec commented on a change in pull request #1293:
URL: https://github.com/apache/hadoop-ozone/pull/1293#discussion_r467139013
##########
File path: hadoop-hdds/common/src/test/java/org/apache/hadoop/ozone/audit/TestOzoneAuditLogger.java
##########
@@ -158,20 +184,34 @@ private void verifyLog(String expected) throws IOException {
}
i++;
}
-
- // When log entry is expected, the log file will contain one line and
- // that must be equal to the expected string
- assertTrue(lines.size() != 0);
- assertTrue(expected.equalsIgnoreCase(lines.get(0)));
+ //check if every expected string can be found in the log entry
+ for(String expected : expectedStrings) {
+ assertTrue(contains(lines, expected));
Review comment:
Added hamcrest suggestion. I find the legibility of hamcrest messages useful especially for evaluating log entries
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] fapifta commented on a change in pull request #1293: HDDS-1889 Add support for verifying multiline log entry
Posted by GitBox <gi...@apache.org>.
fapifta commented on a change in pull request #1293:
URL: https://github.com/apache/hadoop-ozone/pull/1293#discussion_r466969810
##########
File path: hadoop-hdds/common/src/test/java/org/apache/hadoop/ozone/audit/TestOzoneAuditLogger.java
##########
@@ -143,7 +143,31 @@ public void notLogReadEvents() throws IOException {
verifyNoLog();
}
- private void verifyLog(String expected) throws IOException {
+ /**
+ * Test to verify if multiline entries can be checked.
+ */
+
+ @Test
+ public void messageIncludesMultilineException() throws IOException {
+ String exceptionMessage = "Dummuy exception message";
Review comment:
there is a typo here in the string Dummuy ;)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org