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

[MINOR] test: fix tempdir leak in KerberizedHdfs tests #721

Merged
merged 2 commits into from
Mar 15, 2023

Conversation

kaijchen
Copy link
Contributor

What changes were proposed in this pull request?

Use JUnit 5 managed @TempDir in KerberizedHdfs test.

Why are the changes needed?

The tempdir created by KerberizedHdfs is leaking.
It may cause test failures in some condition.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Before:

$ mvn test -Dtest=HadoopFilesystemProviderTest
...
$ find . -name 'serverKS.jks'
./common/target/tmp/kerberizedDfsBaseDir1821275617120252168/serverKS.jks

After:

$ mvn test -Dtest=HadoopFilesystemProviderTest
...
$ find . -name 'serverKS.jks'

@kaijchen kaijchen changed the title [MINOR] test: fix tempdir leak in KerberizedHdfs [MINOR] test: fix tempdir leak in KerberizedHdfs tests Mar 15, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 15, 2023

Codecov Report

Merging #721 (8d3010b) into master (1e605c7) will increase coverage by 2.31%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #721      +/-   ##
============================================
+ Coverage     60.49%   62.81%   +2.31%     
- Complexity     1846     1850       +4     
============================================
  Files           228      215      -13     
  Lines         12744    10785    -1959     
  Branches       1067     1064       -3     
============================================
- Hits           7710     6775     -935     
+ Misses         4625     3660     -965     
+ Partials        409      350      -59     

see 30 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kaijchen
Copy link
Contributor Author

@zuston please cherry-pick this to branch-0.7 after it's merged.

Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

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

LGTM

@jerqi jerqi merged commit 37d7659 into apache:master Mar 15, 2023
jerqi pushed a commit that referenced this pull request Mar 15, 2023
### What changes were proposed in this pull request?

Use JUnit 5 managed `@TempDir` in `KerberizedHdfs` test.

### Why are the changes needed?

The tempdir created by `KerberizedHdfs` is leaking.
It may cause test failures in some condition.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Before:

```console
$ mvn test -Dtest=HadoopFilesystemProviderTest
...
$ find . -name 'serverKS.jks'
./common/target/tmp/kerberizedDfsBaseDir1821275617120252168/serverKS.jks
```

After:

```console
$ mvn test -Dtest=HadoopFilesystemProviderTest
...
$ find . -name 'serverKS.jks'
```
@jerqi
Copy link
Contributor

jerqi commented Mar 15, 2023

Merge to master & branch 0.7.

@kaijchen
Copy link
Contributor Author

Thanks @jerqi for the review.

@kaijchen kaijchen deleted the tempdir-leak branch March 15, 2023 06:18
@jerqi
Copy link
Contributor

jerqi commented Mar 16, 2023

@kaijchen
We run

mvn package

The test fail
ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 1.896 s <<< FAILURE! - in org.apache.uniffle.common.filesystem.HadoopFilesystemProviderTest

[ERROR] org.apache.uniffle.common.filesystem.HadoopFilesystemProviderTest Time elapsed: 1.896 s <<< ERROR!

java.lang.Exception: java.io.FileNotFoundException: /Users/roryqi/Downloads/apache-uniffle-0.7.0-incubating-src/common/target/tmp/junit1553702532156578735/serverKS.jks (No such file or directory)

I revert this pr. The tests passed.

@kaijchen
Copy link
Contributor Author

@jerqi OK, please revert this. I'll look into it later.

jerqi added a commit that referenced this pull request Mar 16, 2023
jerqi added a commit that referenced this pull request Mar 16, 2023
jerqi added a commit that referenced this pull request Mar 16, 2023
advancedxy pushed a commit to advancedxy/incubator-uniffle that referenced this pull request Mar 21, 2023
### What changes were proposed in this pull request?

Use JUnit 5 managed `@TempDir` in `KerberizedHdfs` test.

### Why are the changes needed?

The tempdir created by `KerberizedHdfs` is leaking.
It may cause test failures in some condition.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Before:

```console
$ mvn test -Dtest=HadoopFilesystemProviderTest
...
$ find . -name 'serverKS.jks'
./common/target/tmp/kerberizedDfsBaseDir1821275617120252168/serverKS.jks
```

After:

```console
$ mvn test -Dtest=HadoopFilesystemProviderTest
...
$ find . -name 'serverKS.jks'
```
advancedxy pushed a commit to advancedxy/incubator-uniffle that referenced this pull request Mar 21, 2023
xianjingfeng pushed a commit to xianjingfeng/incubator-uniffle that referenced this pull request Mar 26, 2023
### What changes were proposed in this pull request?

Use JUnit 5 managed `@TempDir` in `KerberizedHdfs` test.

### Why are the changes needed?

The tempdir created by `KerberizedHdfs` is leaking.
It may cause test failures in some condition.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Before:

```console
$ mvn test -Dtest=HadoopFilesystemProviderTest
...
$ find . -name 'serverKS.jks'
./common/target/tmp/kerberizedDfsBaseDir1821275617120252168/serverKS.jks
```

After:

```console
$ mvn test -Dtest=HadoopFilesystemProviderTest
...
$ find . -name 'serverKS.jks'
```
xianjingfeng pushed a commit to xianjingfeng/incubator-uniffle that referenced this pull request Mar 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants