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

Suppress NoSuchFileException in LocalFileIterator #20526

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Feb 1, 2024

Description

Files.walk method may throw an exception during the iteration. https://bugs.openjdk.org/browse/JDK-8039910
Fixes #20520

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

@cla-bot cla-bot bot added the cla-signed label Feb 1, 2024
@ebyhr ebyhr changed the title Suppress NoSuchFileException in FileHiveMetastore.java and LocalFileIterator Suppress NoSuchFileException in FileHiveMetastore and LocalFileIterator Feb 1, 2024
@github-actions github-actions bot added tests:hive hive Hive connector labels Feb 1, 2024
@ebyhr ebyhr force-pushed the ebi/iceberg-flaky-test branch from 72a91bb to 0d4cb0d Compare February 1, 2024 02:46
@ebyhr ebyhr self-assigned this Feb 1, 2024
@ebyhr ebyhr requested review from dain and findepi February 1, 2024 04:37
@findepi findepi added the no-release-notes This pull request does not require release notes entry label Feb 1, 2024
@dain dain requested review from electrum and removed request for dain February 1, 2024 20:24
@dain
Copy link
Member

dain commented Feb 1, 2024

Please get @electrum to review this before merging. The exact behavior of exceptions from the file system implementations is required to be consistent, and I'd like david to make sure this is following the expected behavior.

Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

The Javadoc for listFiles() says

If the location does not exist, an empty iterator is returned.

For hierarchical file systems, if the path is not a directory, an exception is raised.

The expectation is that it only throws in the above case, or if there is a network error or similar. We should never see NoSuchFileException in FileHiveMetastore for listFiles(). If we do, that's a bug in the file system implementation.

@electrum
Copy link
Member

electrum commented Feb 1, 2024

So I believe that the first commit is correct and the second commit is not needed.

@ebyhr
Copy link
Member Author

ebyhr commented Feb 2, 2024

@electrum Both commits are required to fix the concurrent issue. LocalFileIterator#next calls Files.size(path). It throws NoSuchFileException if the file is deleted concurrently.

@electrum
Copy link
Member

electrum commented Feb 2, 2024

Thanks for explaining. We should change LocalFileIterator to ignore those exceptions by not returning the file entry. Listing files is not atomic, so this is an expected race condition that we need to handle.

@findinpath
Copy link
Contributor

We should change LocalFileIterator to ignore those exceptions by not returning the file entry.

@ebyhr did you consider making use of a simplified version of java.nio.file.Files#walkFileTree(java.nio.file.Path, java.util.Set<java.nio.file.FileVisitOption>, int, java.nio.file.FileVisitor<? super java.nio.file.Path>) (what Files.walkFileTree uses in the background) and create the FileEntry instances on the fly in the LocalFileIterator?

@ebyhr ebyhr force-pushed the ebi/iceberg-flaky-test branch from 0d4cb0d to 26e0ff7 Compare February 16, 2024 06:10
@ebyhr ebyhr changed the title Suppress NoSuchFileException in FileHiveMetastore and LocalFileIterator Suppress NoSuchFileException in LocalFileIterator Feb 16, 2024
@ebyhr ebyhr requested a review from electrum February 16, 2024 06:10
@ebyhr ebyhr merged commit dd49451 into trinodb:master Feb 21, 2024
64 checks passed
@ebyhr ebyhr deleted the ebi/iceberg-flaky-test branch February 21, 2024 05:42
@github-actions github-actions bot added this to the 440 milestone Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed hive Hive connector no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

Flaky TestIcebergConnectorSmokeTest.testCreateSchema
5 participants