[FLINK-9920] Only check part files in BucketingSinkFaultToleranceITCase #7639
Conversation
Before, it could happen that other files are in the same directory. Then the verification logic in the test would pick up those files and the test would fail. Now we ignore all files that don't match our expected pattern.
|
cc @Myasuka |
|
Before, it could happen that other files are in the same directory. Then |
There was a problem hiding this comment.
Even we could make BucketingSinkFaultToleranceITCase not failing again, I think we should also clean up the test file created in RollingSink#reflectTruncate and BucketingSink#reflectTruncate before throwing the RuntimeException out, instead of just log error Could not create file for checking if truncate works.
| // if (!file.getPath().getName().startsWith(PART_PREFIX)) { | ||
| // ignore files that don't match with our expected part prefix | ||
| // continue; | ||
| // } |
There was a problem hiding this comment.
commented code should be removed
There was a problem hiding this comment.
Ah, this is the code I actually added. I must have commented out by accident.
|
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. Bot commandsThe @flinkbot bot supports the following commands:
|
|
@Myasuka I wrapped the whole checking logic in a |
| "Could not create file for checking if truncate works. " + | ||
| "You can disable support for truncate() completely via " + | ||
| "BucketingSink.setUseTruncate(false).", | ||
| e); |
There was a problem hiding this comment.
Nit: Move the e back to the previous line for uniformity.
| fs.delete(testPath, false); | ||
| } catch (IOException e) { | ||
| LOG.error("Could not delete truncate test file.", e); | ||
| throw new RuntimeException("Could not delete truncate test file. " + |
There was a problem hiding this comment.
I am not sure if we want to fail the job if we cannot delete the test file.
I would say it is enough to just log the fact and move on. If there is a bigger problem and this was not just a hick-up, the job will fail later when something that matters fails (e.g. we cannot create a tmp file).
What do you think @aljoscha ?
There was a problem hiding this comment.
I agree, but I want to keep this change minimal. The decision a while back was to throw this exception and have users manually disable truncate if they want to get rid of the exception.
| LOG.debug("Truncate is not supported.", e); | ||
| m = null; | ||
| } | ||
| try (FSDataOutputStream outputStream = fs.create(testPath)) { |
|
Merged |
|
Sorry for not replying late, we just had the Chinese new year. LGTM |
|
No problem @Myasuka 😃 |
No description provided.