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/12 08:22:32 UTC
[GitHub] [hadoop-ozone] smengcl opened a new pull request #1316: HDDS-4109. Tests in TestOzoneFileSystem should use the existing MiniOzoneCluster
smengcl opened a new pull request #1316:
URL: https://github.com/apache/hadoop-ozone/pull/1316
## What changes were proposed in this pull request?
4 new tests have been added since HDDS-2883 and are not sharing that MiniOzoneCluster.
I am able to cut down the run time of TestOzoneFileSystem from 3m18s to 1m2s on my Mac. It would only save more run time on GitHub Workflow.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4109
## How was this patch tested?
No new tests needed.
----------------------------------------------------------------
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] ayushtkn commented on a change in pull request #1316: HDDS-4109. Tests in TestOzoneFileSystem should use the existing MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
ayushtkn commented on a change in pull request #1316:
URL: https://github.com/apache/hadoop-ozone/pull/1316#discussion_r478571715
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java
##########
@@ -154,21 +155,21 @@ public void testCreateFileShouldCheckExistenceOfDirWithSameName()
} catch (FileAlreadyExistsException fae) {
// ignore as its expected
}
+
+ // Cleanup
+ fs.delete(new Path("/d1/"), true);
Review comment:
Shouldn't the cleanup be in a `finally` block? post `mkdir` if the test fails the directory won't get cleaned up. Wouldn't that affect other tests?
----------------------------------------------------------------
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] smengcl commented on a change in pull request #1316: HDDS-4109. Tests in TestOzoneFileSystem should use the existing MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1316:
URL: https://github.com/apache/hadoop-ozone/pull/1316#discussion_r478623689
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java
##########
@@ -154,21 +155,21 @@ public void testCreateFileShouldCheckExistenceOfDirWithSameName()
} catch (FileAlreadyExistsException fae) {
// ignore as its expected
}
+
+ // Cleanup
+ fs.delete(new Path("/d1/"), true);
Review comment:
Thanks for taking a look at this @ayushtkn .
In this case it doesn't matter IMO. Cause if this test (`testCreateFileShouldCheckExistenceOfDirWithSameName`) fails and throws, `testFileSystem` as a whole will fail at this point thus the tests following this one won't even be executed.
----------------------------------------------------------------
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] smengcl merged pull request #1316: HDDS-4109. Tests in TestOzoneFileSystem should use the existing MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
smengcl merged pull request #1316:
URL: https://github.com/apache/hadoop-ozone/pull/1316
----------------------------------------------------------------
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] smengcl commented on a change in pull request #1316: HDDS-4109. Tests in TestOzoneFileSystem should use the existing MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1316:
URL: https://github.com/apache/hadoop-ozone/pull/1316#discussion_r478703575
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java
##########
@@ -154,21 +155,21 @@ public void testCreateFileShouldCheckExistenceOfDirWithSameName()
} catch (FileAlreadyExistsException fae) {
// ignore as its expected
}
+
+ // Cleanup
+ fs.delete(new Path("/d1/"), true);
Review comment:
Filed https://issues.apache.org/jira/browse/HDDS-4162
----------------------------------------------------------------
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] smengcl commented on a change in pull request #1316: HDDS-4109. Tests in TestOzoneFileSystem should use the existing MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1316:
URL: https://github.com/apache/hadoop-ozone/pull/1316#discussion_r478623689
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java
##########
@@ -154,21 +155,21 @@ public void testCreateFileShouldCheckExistenceOfDirWithSameName()
} catch (FileAlreadyExistsException fae) {
// ignore as its expected
}
+
+ // Cleanup
+ fs.delete(new Path("/d1/"), true);
Review comment:
Thanks for taking a look at this @ayushtkn .
In this case it doesn't matter IMO. Cause if this test (`testCreateFileShouldCheckExistenceOfDirWithSameName`) fails and throws, `testFileSystem` as a whole will fail at this point thus the consecutive tests won't be executed.
----------------------------------------------------------------
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] smengcl commented on pull request #1316: HDDS-4109. Tests in TestOzoneFileSystem should use the existing MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #1316:
URL: https://github.com/apache/hadoop-ozone/pull/1316#issuecomment-680128921
> Thanks @smengcl for this speedup. Can you please add a comment that new tests should be added as private methods invoked from `testFileSystem()` to help avoid regression next time?
Sure thing. I've added a note in the javadoc of 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] adoroszlai commented on pull request #1316: HDDS-4109. Tests in TestOzoneFileSystem should use the existing MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #1316:
URL: https://github.com/apache/hadoop-ozone/pull/1316#issuecomment-672728306
> 4 new tests have been added since [HDDS-2883](https://issues.apache.org/jira/browse/HDDS-2883) and are not sharing that MiniOzoneCluster.
I think it's HDDS-2833.
----------------------------------------------------------------
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] smengcl commented on pull request #1316: HDDS-4109. Tests in TestOzoneFileSystem should use the existing MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #1316:
URL: https://github.com/apache/hadoop-ozone/pull/1316#issuecomment-682198590
Thanks @ayushtkn for reviewing this. I have filed https://issues.apache.org/jira/browse/HDDS-4162 for the follow-up refactoring (use `BeforeClass`).
Will merge shortly.
----------------------------------------------------------------
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] smengcl commented on pull request #1316: HDDS-4109. Tests in TestOzoneFileSystem should use the existing MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
smengcl commented on pull request #1316:
URL: https://github.com/apache/hadoop-ozone/pull/1316#issuecomment-672748710
> > 4 new tests have been added since [HDDS-2883](https://issues.apache.org/jira/browse/HDDS-2883) and are not sharing that MiniOzoneCluster.
>
> I think it's [HDDS-2833](https://issues.apache.org/jira/browse/HDDS-2833).
Fixed typo. Thanks!
----------------------------------------------------------------
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] smengcl commented on a change in pull request #1316: HDDS-4109. Tests in TestOzoneFileSystem should use the existing MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
smengcl commented on a change in pull request #1316:
URL: https://github.com/apache/hadoop-ozone/pull/1316#discussion_r478700729
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java
##########
@@ -154,21 +155,21 @@ public void testCreateFileShouldCheckExistenceOfDirWithSameName()
} catch (FileAlreadyExistsException fae) {
// ignore as its expected
}
+
+ // Cleanup
+ fs.delete(new Path("/d1/"), true);
Review comment:
Yes, I also think `@BeforeClass` is better. Maybe another jira to refactor the test class this way.
----------------------------------------------------------------
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] ayushtkn commented on a change in pull request #1316: HDDS-4109. Tests in TestOzoneFileSystem should use the existing MiniOzoneCluster
Posted by GitBox <gi...@apache.org>.
ayushtkn commented on a change in pull request #1316:
URL: https://github.com/apache/hadoop-ozone/pull/1316#discussion_r478634376
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/TestOzoneFileSystem.java
##########
@@ -154,21 +155,21 @@ public void testCreateFileShouldCheckExistenceOfDirWithSameName()
} catch (FileAlreadyExistsException fae) {
// ignore as its expected
}
+
+ // Cleanup
+ fs.delete(new Path("/d1/"), true);
Review comment:
hmm, correct the following tests will fail. Do you think, thats a good behaviour, Ideally only the affected test should fail. If there is a common cluster, that could have been initialised in a `BeforeClass` and all other tests could have ran independently.
Anyway not related to what you are doing here. Thanx for confirming.
----------------------------------------------------------------
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