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.

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

[HUDI-4780] Improve test setup #6704

Closed
wants to merge 11 commits into from
Closed

Conversation

loukey-lj
Copy link
Contributor

@loukey-lj loukey-lj commented Sep 17, 2022

Change Logs

this pr supplements the 6602 test case.
TestHoodieLogFormat initialization of fs is inconsistent with the actual running code, this is why the test code is as expected, but the log file is not segmented when actually running.

Impact

Correct the test code to align the test case with the actual running code

Contributor's checklist

  • Read through contributor's guide
  • Change Logs and Impact were stated clearly
  • Adequate tests were added if applicable
  • CI passed

Copy link
Contributor

@yihua yihua left a comment

Choose a reason for hiding this comment

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

@loukey-lj does this add a test for #6602 (HUDI-4780)?

@loukey-lj
Copy link
Contributor Author

@loukey-lj does this add a test for #6602 (HUDI-4780)?

yes

@hudi-bot
Copy link

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

Comment on lines +128 to +134
FileSystem fileSystemOriginal = MiniClusterUtil.fileSystem;

assertTrue(fs.mkdirs(new Path(tempDir.toAbsolutePath().toString())));
assertTrue(fileSystemOriginal.mkdirs(new Path(tempDir.toAbsolutePath().toString())));
this.partitionPath = new Path(tempDir.toAbsolutePath().toString());
this.basePath = tempDir.getParent().toString();
HoodieTestUtils.init(MiniClusterUtil.configuration, basePath, HoodieTableType.MERGE_ON_READ);
HoodieTableMetaClient init = HoodieTestUtils.init(MiniClusterUtil.configuration, basePath, HoodieTableType.MERGE_ON_READ);
this.fs = init.getFs();
Copy link
Contributor

Choose a reason for hiding this comment

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

@loukey-lj Could you clarify how the changes improve the tests for #6602 you put up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the actual running process fs comes from TableMateClinet.So I initialized a TableMateClinetto get fs instead of fileSystemOriginal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Could you point out which set of tests fail without your previous fix (#6602) after this PR? We should add some comments on these tests mentioning that they test the logic around creating new log files based the size configured. If there are no such test, we should add one. Let me know if this makes sense. We can sync up through Hudi Slack.

Copy link
Member

@xushiyan xushiyan left a comment

Choose a reason for hiding this comment

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

can you clarify what improvement this brings? does it solve any flakiness? this does not warrant a new jira. please use the same jira ticket. also please fill the PR description according to the template

@xushiyan xushiyan changed the title [HUDI-4869] Fix test for HUDI-4780 [HUDI-4780] improve test setup Sep 19, 2022
@loukey-lj
Copy link
Contributor Author

@xushiyan thank u,this pr supplements the 6602 test case. You can first look at the review record of 6602 .

@yihua yihua changed the title [HUDI-4780] improve test setup [HUDI-4780] Improve test setup Sep 22, 2022
@yihua yihua added the priority:minor everything else; usability gaps; questions; feature reqs label Sep 22, 2022
@nsivabalan
Copy link
Contributor

@loukey-lj : is this patch still valid ? can you respond to Ethan's and raymond's clarification above.

@nsivabalan nsivabalan added the release-0.12.2 Patches targetted for 0.12.2 label Dec 6, 2022
@codope codope added test-coverage and removed release-0.12.2 Patches targetted for 0.12.2 labels Dec 7, 2022
@github-actions github-actions bot added the size:XS PR with lines of changes in <= 10 label Feb 26, 2024
@yihua
Copy link
Contributor

yihua commented Mar 3, 2024

On the latest master, the file system instance is already instantiated properly in the setup of TestHoodieLogFormat. Closing this PR. @loukey-lj feel free to reopen it when you see fit.

@yihua yihua closed this Mar 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:minor everything else; usability gaps; questions; feature reqs size:XS PR with lines of changes in <= 10 test-coverage
Projects
Status: 🏁 Triaged
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

6 participants