-
Notifications
You must be signed in to change notification settings - Fork 2.5k
[MINOR] Use try with resource to close stream #8596
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
Conversation
| @Test | ||
| public void testOverwriteHoodiePropertiesWithInputStreamFix() throws IOException { | ||
| RepairsCommand command = new RepairsCommand(); | ||
| URL newProps = this.getClass().getClassLoader().getResource("table-config.properties"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the test items are similiar with testOverwriteHoodieProperties ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, their general test logic are similar."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference, can you explain the test purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of the test is to call this method and check whether the inputStream is working properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to call this method
Which method is called here, does testOverwriteHoodieProperties already cover the code path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So testOverwriteHoodieProperties already covers this, can you just take an offline validation instead, we can get rid of these UTs if manual test passes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry,I don't understand how to validate,and how to get rid of these UTs,can you help me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get rid of these UTs
I mean there is no need to add UTs, instead, validate these two commds you have modified offline to make sure it works correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK,understand!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I delete the redundant UTs,and run the exists UTs to validate the commands,it works correctly.
hudi-cli/src/test/java/org/apache/hudi/cli/commands/TestUnclosedIO.java
Outdated
Show resolved
Hide resolved
hudi-cli/src/test/java/org/apache/hudi/cli/commands/TestTableCommand.java
Outdated
Show resolved
Hide resolved
hudi-cli/src/test/java/org/apache/hudi/cli/commands/TestTableCommand.java
Show resolved
Hide resolved
|
The CI is failing, can you check it. |
yeah,found the problem. |
| @ShellOption(value = {"--repairedOutputPath"}, help = "Location to place the repaired files") | ||
| final String repairedOutputPath, | ||
| @ShellOption(value = {"--duplicatedPartitionPath"}, defaultValue = "", help = "Partition Path containing the duplicates") final String duplicatedPartitionPath, | ||
| @ShellOption(value = {"--repairedOutputPath"}, help = "Location to place the repaired files") final String repairedOutputPath, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why we do these format changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why we do these format changes
I used the shortcut keys in IntelliJ IDEA to format it.
|
The failure is known to be flaky, would merge it soon ~ |
Change Logs
use try with resource to close stream
Impact
If input streams are not properly closed after use, it can potentially affect the normal execution of commands or programs.
Risk level (write none, low medium or high below)
there is a low level
Documentation Update
Describe any necessary documentation update if there is any new feature, config, or user-facing change
ticket number here and follow the instruction to make
changes to the website.
Contributor's checklist