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

ARROW-17794: [Java] Force delete jni lib file on JVM exit #14189

Merged
merged 3 commits into from Sep 26, 2022

Conversation

jackylee-ch
Copy link
Contributor

@jackylee-ch jackylee-ch commented Sep 21, 2022

Use File.deleteOnExit to delete jni lib file on JVM exit. File.deleteOnExit actually add a shut down hook to make sure file delte.

@github-actions
Copy link

@github-actions
Copy link

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@lidavidm
Copy link
Member

CC @davisusanibar

@@ -29,6 +29,8 @@
import java.util.List;
import java.util.Set;

import org.apache.commons.io.FileUtils;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is not needed to add commons-io as a Dataset module dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your attension, I have change the way to delete file. No need to use FileUtils as it is a file.

@@ -79,6 +79,7 @@ private void load(String name) {
final String libraryToLoad = System.mapLibraryName(name);
try {
File temp = File.createTempFile("jnilib-", ".tmp", new File(System.getProperty("java.io.tmpdir")));
temp.deleteOnExit();
Copy link
Contributor

Choose a reason for hiding this comment

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

Test: mvn -Darrow.cpp.build.dir=/Users/arrow/lib -Dtest="TestFileSystemDataset#testBaseOrcRead" clean test

Before the changes:

  • target/jnilib-11079005003293013490.tmp
  • target/jnilib-9915730910284592509.tmp

After the changes (1 tmp was deleted):

  • target/jnilib-9915730910284592509.tmp

Could be possible to force delete also the another JNI file created?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can check and resolve this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank for your double check. I have remove the extra lib file and checked it in my environment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, thank you a lot

@jackylee-ch
Copy link
Contributor Author

CC @davisusanibar

Copy link
Contributor

@davisusanibar davisusanibar left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Thanks!

@lidavidm lidavidm merged commit 67f5ac8 into apache:master Sep 26, 2022
@ursabot
Copy link

ursabot commented Sep 26, 2022

Benchmark runs are scheduled for baseline = be30611 and contender = 67f5ac8. 67f5ac8 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.41% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.55% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.11% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 67f5ac87 ec2-t3-xlarge-us-east-2
[Failed] 67f5ac87 test-mac-arm
[Failed] 67f5ac87 ursa-i9-9960x
[Finished] 67f5ac87 ursa-thinkcentre-m75q
[Finished] be306118 ec2-t3-xlarge-us-east-2
[Finished] be306118 test-mac-arm
[Failed] be306118 ursa-i9-9960x
[Finished] be306118 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@jackylee-ch jackylee-ch deleted the delete_lib_on_exit branch September 27, 2022 01:26
@kou
Copy link
Member

kou commented Sep 29, 2022

@github-actions crossbow submit java-jars

@github-actions
Copy link

Revision: a0a821f

Submitted crossbow builds: ursacomputing/crossbow @ actions-727dfc5f94

Task Status
java-jars Github Actions

zagto pushed a commit to zagto/arrow that referenced this pull request Oct 7, 2022
Use `File.deleteOnExit` to delete jni lib file on JVM exit. `File.deleteOnExit` actually add a shut down hook to make sure file delte.

Authored-by: jackylee-ch <lijunqing@baidu.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
fatemehp pushed a commit to fatemehp/arrow that referenced this pull request Oct 17, 2022
Use `File.deleteOnExit` to delete jni lib file on JVM exit. `File.deleteOnExit` actually add a shut down hook to make sure file delte.

Authored-by: jackylee-ch <lijunqing@baidu.com>
Signed-off-by: David Li <li.davidm96@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants