You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "wzhallright (via GitHub)" <gi...@apache.org> on 2024/01/30 01:56:23 UTC
[PR] HDDS-10164. Replace GenericTestUtils temp dirs with `@TempDir` in hadoop-hdds [ozone]
wzhallright opened a new pull request, #6122:
URL: https://github.com/apache/ozone/pull/6122
## What changes were proposed in this pull request?
Replace temporary directory functionality of GenericTestUtils with JUnit's [@TempDir](https://junit.org/junit5/docs/current/user-guide/#writing-tests-built-in-extensions-TempDirectory). The latter is automatically cleaned up.
* Usage of File getRandomizedTestDir(), File getTestDir(), and File getTestDir(String) can be directly replaced by @TempDir. If the temp directory is stored in a local variable, convert it to a field for it.
* For usages of String getRandomizedTempPath() and String getTempPath(String) we still need to add a Path or File as a field, and use toString() where needed.
* Post-test deletion of the dir can be removed.
* If @BeforeEach and @AfterEach methods are only used to create/delete the dir, they can be deleted as well.
* Any unused imports should be removed.
## What is the link to the Apache JIRA
[HDDS-10164](https://issues.apache.org/jira/browse/HDDS-10164)
## CI
https://github.com/wzhallright/ozone/actions/runs/7694859609
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10164. Replace GenericTestUtils temp dirs with `@TempDir` in hadoop-hdds [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #6122:
URL: https://github.com/apache/ozone/pull/6122
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10164. Replace GenericTestUtils temp dirs with `@TempDir` in hadoop-hdds [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #6122:
URL: https://github.com/apache/ozone/pull/6122#discussion_r1471049423
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestCapacityVolumeChoosingPolicy.java:
##########
@@ -94,9 +102,9 @@ public void setup() throws Exception {
@AfterEach
public void cleanUp() {
volumes.forEach(HddsVolume::shutdown);
- FileUtil.fullyDelete(new File(VOLUME_1));
- FileUtil.fullyDelete(new File(VOLUME_2));
- FileUtil.fullyDelete(new File(VOLUME_3));
+ FileUtil.fullyDelete(new File(volume1));
+ FileUtil.fullyDelete(new File(volume2));
+ FileUtil.fullyDelete(new File(volume3));
Review Comment:
Making `baseDir` non-`static`, and initializing `volumeX` in `@BeforeEach`, we could remove the need for `FileUtil.fullyDelete` in `@AfterEach`.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/SCMTestUtils.java:
##########
@@ -52,6 +52,9 @@
* Test Endpoint class.
*/
public final class SCMTestUtils {
+
+ @TempDir
+ private static File testRoot;
Review Comment:
`@TempDir` does not work here, since `SCMTestUtils` is not a test class, so `testRoot` will be `null`.
Let's address this in a separate task (HDDS-10243), keep `SCMTestUtils` unchanged in this PR.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestHddsDispatcher.java:
##########
@@ -108,13 +108,12 @@ public class TestHddsDispatcher {
@ContainerLayoutTestInfo.ContainerTest
public void testContainerCloseActionWhenFull(
- ContainerLayoutVersion layout) throws IOException {
+ ContainerLayoutVersion layout, @TempDir File testDir) throws IOException {
Review Comment:
This works fine, but we could simplify by adding it as member variable instead of parameter in each method.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/volume/TestRoundRobinVolumeChoosingPolicy.java:
##########
@@ -50,11 +52,16 @@ public class TestRoundRobinVolumeChoosingPolicy {
private final List<HddsVolume> volumes = new ArrayList<>();
private static final OzoneConfiguration CONF = new OzoneConfiguration();
- private static final String BASE_DIR =
- getTestDir(TestRoundRobinVolumeChoosingPolicy.class.getSimpleName())
- .getAbsolutePath();
- private static final String VOLUME_1 = BASE_DIR + "disk1";
- private static final String VOLUME_2 = BASE_DIR + "disk2";
+ @TempDir
+ private static Path baseDir;
+ private static String volume1;
+ private static String volume2;
Review Comment:
Same here: please change these to non-`static` and remove manual delete in `@AfterEach`.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/pipeline/TestRatisPipelineProvider.java:
##########
@@ -89,33 +89,33 @@ public void init(int maxPipelinePerNode) throws Exception {
public void init(int maxPipelinePerNode, OzoneConfiguration conf)
throws Exception {
- testDir = GenericTestUtils.getTestDir(
- TestRatisPipelineProvider.class.getSimpleName() + UUID.randomUUID());
- conf.set(HddsConfigKeys.OZONE_METADATA_DIRS, testDir.getAbsolutePath());
+ init(maxPipelinePerNode, conf, testDir);
+ }
+
+ public void init(int maxPipelinePerNode, OzoneConfiguration conf, File dir) throws Exception {
+ conf.set(HddsConfigKeys.OZONE_METADATA_DIRS, dir.getAbsolutePath());
dbStore = DBStoreBuilder.createDBStore(
- conf, new SCMDBDefinition());
+ conf, new SCMDBDefinition());
nodeManager = new MockNodeManager(true, 10);
nodeManager.setNumPipelinePerDatanode(maxPipelinePerNode);
SCMHAManager scmhaManager = SCMHAManagerStub.getInstance(true);
conf.setInt(ScmConfigKeys.OZONE_DATANODE_PIPELINE_LIMIT,
- maxPipelinePerNode);
+ maxPipelinePerNode);
stateManager = PipelineStateManagerImpl.newBuilder()
- .setPipelineStore(SCMDBDefinition.PIPELINES.getTable(dbStore))
- .setRatisServer(scmhaManager.getRatisServer())
- .setNodeManager(nodeManager)
- .setSCMDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
- .build();
+ .setPipelineStore(SCMDBDefinition.PIPELINES.getTable(dbStore))
+ .setRatisServer(scmhaManager.getRatisServer())
+ .setNodeManager(nodeManager)
+ .setSCMDBTransactionBuffer(scmhaManager.getDBTransactionBuffer())
+ .build();
provider = new MockRatisPipelineProvider(nodeManager,
- stateManager, conf);
+ stateManager, conf);
Review Comment:
nit: let's keep the original indentation here.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-10164. Replace GenericTestUtils temp dirs with `@TempDir` in hadoop-hdds [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #6122:
URL: https://github.com/apache/ozone/pull/6122#issuecomment-1918818498
Thanks @wzhallright for the contribution.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org