Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add test for DoraMetaManager #18127

Merged
merged 26 commits into from
Sep 26, 2023
Merged

Add test for DoraMetaManager #18127

merged 26 commits into from
Sep 26, 2023

Conversation

voddle
Copy link
Contributor

@voddle voddle commented Sep 11, 2023 •
edited by YichuanSun
Loading

Add DoraMetaManagerTest.

YichuanSun
YichuanSun suggested changes Sep 14, 2023
View reviewed changes
Copy link
Contributor

@YichuanSun YichuanSun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I will leave some comments :)

dora/core/server/worker/src/test/java/alluxio/worker/netty/WriteRequestTest.java Outdated Show resolved Hide resolved
dora/core/server/worker/src/test/java/alluxio/worker/netty/UnsupportedMessageHandlerTest.java Outdated Show resolved Hide resolved
dora/core/server/worker/src/test/java/alluxio/worker/netty/DoraMetaManagerTest.java Outdated Show resolved Hide resolved
dora/core/server/worker/src/test/java/alluxio/worker/netty/DoraMetaManagerTest.java Outdated Show resolved Hide resolved
dora/core/server/worker/src/test/java/alluxio/worker/netty/DoraMetaManagerTest.java Outdated Show resolved Hide resolved
dora/core/server/worker/src/test/java/alluxio/worker/netty/DoraMetaManagerTest.java Outdated Show resolved Hide resolved
dora/core/server/worker/src/test/java/alluxio/worker/netty/DoraMetaManagerTest.java Outdated Show resolved Hide resolved
dora/core/server/worker/src/test/java/alluxio/worker/netty/DoraMetaManagerTest.java Outdated Show resolved Hide resolved
dora/core/server/worker/src/test/java/alluxio/worker/netty/DoraMetaManagerTest.java Outdated Show resolved Hide resolved
dora/core/server/worker/src/test/java/alluxio/worker/netty/DoraMetaManagerTest.java Outdated Show resolved Hide resolved
YichuanSun
YichuanSun reviewed Sep 18, 2023
View reviewed changes
Copy link
Contributor

@YichuanSun YichuanSun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Voddle, I leave some comments. :)

dora/core/server/worker/src/test/java/alluxio/worker/netty/UfsFileWriteHandlerTest.java Outdated Show resolved Hide resolved
dora/core/server/worker/src/test/java/alluxio/worker/dora/DoraMetaManagerTest.java Outdated Show resolved Hide resolved
dora/core/server/worker/src/test/java/alluxio/worker/dora/DoraMetaManagerTest.java Outdated Show resolved Hide resolved
@YichuanSun
Copy link
Contributor

BTW, please fix your checkstyle.

@YichuanSun YichuanSun changed the title Add test for WriteRequest Add test for DoraMetaManager Sep 19, 2023
YichuanSun
YichuanSun suggested changes Sep 19, 2023
View reviewed changes
Copy link
Contributor

@YichuanSun YichuanSun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is nearly finished, thanks, I leave some comments.

dora/core/server/worker/src/test/java/alluxio/worker/dora/DoraMetaManagerTest.java Outdated Show resolved Hide resolved
dora/core/server/worker/src/test/java/alluxio/worker/dora/DoraMetaManagerTest.java Outdated
if (testDir.exists()) {
deleteDirectory(testDir);
} else {
throw new RuntimeException("testDir doesn't exist");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the directory is not existing, how about do nothing? I'm not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it not existing, somethings must be wrong 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So now would you mind delete this method? Actually we don't create any files in the directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testDir now is hard coded as 'alluxio/dora/core/worker', without here will leave an annoying directory, or we change it to tmpfile

dora/core/server/worker/src/test/java/alluxio/worker/dora/DoraMetaManagerTest.java Show resolved Hide resolved
dora/core/server/worker/src/test/java/alluxio/worker/dora/DoraMetaManagerTest.java Outdated Show resolved Hide resolved
YichuanSun
YichuanSun suggested changes Sep 19, 2023
View reviewed changes
Copy link
Contributor

@YichuanSun YichuanSun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! these are my last comments I think.

dora/core/server/worker/src/test/java/alluxio/worker/dora/DoraMetaManagerTest.java Outdated
if (testDir.exists()) {
deleteDirectory(testDir);
} else {
throw new RuntimeException("testDir doesn't exist");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So now would you mind delete this method? Actually we don't create any files in the directory.

dora/core/server/worker/src/test/java/alluxio/worker/dora/DoraMetaManagerTest.java Outdated Show resolved Hide resolved
jiacheliu3
jiacheliu3 requested changes Sep 23, 2023
View reviewed changes
Copy link
Contributor

@jiacheliu3 jiacheliu3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM with a few comments. Thanks for the work!

dora/core/server/worker/src/test/java/alluxio/worker/dora/DoraMetaManagerTest.java Outdated
} catch (IOException e) {
mManager = null;
}
File testDir = new File(mTestMetaStorePath);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea use TemporaryFolder, which deletes itself when jvm exits

dora/core/server/worker/src/test/java/alluxio/worker/dora/DoraMetaManagerTest.java
@Before
public void before() throws IOException {
AlluxioProperties prop = new AlluxioProperties();
prop.set(PropertyKey.DORA_WORKER_METASTORE_ROCKSDB_DIR, String.format("%s/metastore",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here utilize the TemporaryFolder you are going to use

jiacheliu3
jiacheliu3 approved these changes Sep 26, 2023
View reviewed changes
Copy link
Contributor

@jiacheliu3 jiacheliu3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@jiacheliu3 jiacheliu3 added the type-code-quality code quality improvement label Sep 26, 2023
@jiacheliu3
Copy link
Contributor

alluxio-bot, merge this please

@alluxio-bot alluxio-bot merged commit 9ab7391 into Alluxio:main Sep 26, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YichuanSun YichuanSun YichuanSun requested changes

@jiacheliu3 jiacheliu3 jiacheliu3 approved these changes

Assignees
No one assigned
Labels
type-code-quality code quality improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@voddle @YichuanSun @jiacheliu3 @alluxio-bot