[BEAM-12740] [BEAM-8268] Improve error handling and retries for GCS rename used by FileBasedSink #15301

scwhittle · 2021-08-10T13:31:09Z

Previously exceptions such as timeouts or unavailable for a single file would not be caught
and would cause retries and backoff of the entire DoFn processing.

Additionally plumb the options to ignore missing sources through to underlying FileSystem.rename
so that file systems can handle such errors intelligently per-file. The filtering in FileSystems.rename
to remove such files is beneficial because it may use cheaper match operations but it is insufficient
as previous deletes observed as timeouts may delete files between the match and rename.

Further reduce operations by modifying FileBasedSink to not delete files that have been renamed.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

`ValidatesRunner` compliance status (on master branch)

Lang	ULR	Twister2
Go	---	---
Java
Python	---	---
XLang		---

Examples testing status on various runners

Lang	ULR	Dataflow	Flink	Samza	Spark	Twister2
Go	---	---	---	---	---	---	---
Java	---		---	---	---	---	---
Python	---	---	---	---	---	---	---
XLang	---	---	---	---	---	---	---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go	Java	Python

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---			---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

chamikaramj

Thanks.

chamikaramj · 2021-08-10T17:42:03Z

sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileBasedSink.java

I think we still need removeTemporaryFiles() to remove the temporary directory for batch.

adding back, I think we need this as well if we are filtering if the destination exists because we still need to remove the source in that case.

Might make sense to update the semantics of "StandardMoveOptions.SKIP_IF_DESTINATION_EXISTS" to delete the source during rename. That will allow us to prevent the double delete for the case where the source existed.

Also probably removeTemporaryFiles() should be updated to just cleanup the temporary directory (where appropriate) instead of trying to delete already renamed files.

I modified FileSystems.rename to delete srcs that existed but were filtered due to dest existing.
I kept the existing methods in FileBasedSink because it appears they are designed to be called by subclasses. I changed to pass an empty set for known files after the rename to avoid the unnecessary delete.

sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileSystems.java

sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileBasedSink.java

...oogle-cloud-platform-core/src/main/java/org/apache/beam/sdk/extensions/gcp/util/GcsUtil.java

scwhittle · 2021-08-11T13:37:54Z

PTAL, I removed the hacks for testing since the performance seemed greatly improved.
Additionally I removed the unnecessary delete of renamed files in FileBasedSink by improving FileSystems.remove to always delete src files (even if the copy is skipped due to dest existing and that being filtered).
I added a unit test for rename by adding support to inject a batch object. The apiary BatchRequest object is difficult to mock so I opted for mocking above that.

scwhittle · 2021-08-11T13:38:12Z

@reuvenlax @chamikaramj

chamikaramj

Thanks. LGTM other than one comment.

chamikaramj · 2021-08-11T18:39:36Z

sdks/java/io/amazon-web-services/src/main/java/org/apache/beam/sdk/io/aws/s3/S3FileSystem.java

We should raise an UnsupportedOperationexception for this and other FileSystems when MoveOptions that are not fully supported are used.

Done, changed FileSystems to catch the exception and retry without move options to keep existing behavior for filesystems that don't support options.

I also fixed bug where I wasn't forwarding on the move options as I thought to the filesystem in FileSystems.rename ( and thus wasn't using the improved error handling in GcsFileSystem)

chamikaramj · 2021-08-11T18:40:42Z

Run Java PreCommit

chamikaramj · 2021-08-11T18:48:42Z

Run Java_Examples_Dataflow PreCommit

chamikaramj · 2021-08-11T18:49:24Z

Run Java_Examples_Dataflow_Java11 PreCommit

chamikaramj · 2021-08-11T18:50:07Z

Not sure why word count failed, but re-trying filed tests.

…ename operations Previously exceptions such as timeouts or unavailable for a single file would not be caught and would cause retries and backoff of the entire DoFn processing. Additionally plumb the options to ignore missing sources through to underlying FileSystem.rename so that file systems can handle such errors intelligently per-file. The filtering in FileSystems.rename to remove such files is beneficial because it may use cheaper match operations but it is insufficient as previous deletes observed as timeouts may delete files between the match and rename. Further reduce operations by modifying FileBasedSink to not delete files that have been renamed.

chamikaramj · 2021-08-11T19:57:25Z

LGTM. Thanks.

chamikaramj · 2021-08-12T06:35:26Z

Run Java PreCommit

scwhittle · 2021-08-12T10:12:18Z

Run Java PreCommit

scwhittle · 2021-08-12T10:12:38Z

Run Java_Examples_Dataflow PreCommit

scwhittle · 2021-08-12T10:12:52Z

Run Java_Examples_Dataflow_Java11 PreCommit

scwhittle · 2021-08-16T12:58:45Z

Run Java PreCommit

scwhittle · 2021-08-16T13:22:00Z

Run Java_Examples_Dataflow PreCommit

scwhittle · 2021-08-16T13:22:18Z

Run Java_Examples_Dataflow_Java11 PreCommit

lukecwik · 2021-08-16T18:44:33Z

Run Java_Examples_Dataflow PreCommit

lukecwik · 2021-08-16T18:44:41Z

Run Java_Examples_Dataflow_Java11 PreCommit

scwhittle · 2021-08-18T08:36:45Z

@chamikaramj @lukecwik Tests are passing now, I think this is ready to merge

chamikaramj · 2021-08-18T17:47:47Z

Thanks. Yeah, we can merge.

chamikaramj reviewed Aug 10, 2021

View reviewed changes

scwhittle force-pushed the gcs_errors branch 2 times, most recently from 8d30e0a to e288ade Compare August 11, 2021 13:19

scwhittle changed the title ~~Change renames to handle retries internally~~ [BEAM-12740] [BEAM-8268] Improve error handling and retries for GCS rename used by FileBasedSink Aug 11, 2021

scwhittle force-pushed the gcs_errors branch from e288ade to 5521cac Compare August 11, 2021 13:28

chamikaramj reviewed Aug 11, 2021

View reviewed changes

scwhittle force-pushed the gcs_errors branch from 5521cac to 17a2e3e Compare August 11, 2021 19:43

chamikaramj merged commit a0fbe00 into apache:master Aug 18, 2021

scwhittle deleted the gcs_errors branch August 30, 2021 09:58

[BEAM-12740] [BEAM-8268] Improve error handling and retries for GCS rename used by FileBasedSink #15301

[BEAM-12740] [BEAM-8268] Improve error handling and retries for GCS rename used by FileBasedSink #15301

Uh oh!

Conversation

scwhittle commented Aug 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ValidatesRunner compliance status (on master branch)

Examples testing status on various runners

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

Uh oh!

chamikaramj left a comment

Choose a reason for hiding this comment

Uh oh!

chamikaramj Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

scwhittle Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

chamikaramj Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

scwhittle Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scwhittle commented Aug 11, 2021

Uh oh!

scwhittle commented Aug 11, 2021

Uh oh!

chamikaramj left a comment

Choose a reason for hiding this comment

Uh oh!

chamikaramj Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

scwhittle Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

chamikaramj commented Aug 11, 2021

Uh oh!

chamikaramj commented Aug 11, 2021

Uh oh!

chamikaramj commented Aug 11, 2021

Uh oh!

chamikaramj commented Aug 11, 2021

Uh oh!

chamikaramj commented Aug 11, 2021

Uh oh!

chamikaramj commented Aug 12, 2021

Uh oh!

scwhittle commented Aug 12, 2021

Uh oh!

scwhittle commented Aug 12, 2021

Uh oh!

scwhittle commented Aug 12, 2021

Uh oh!

scwhittle commented Aug 16, 2021

Uh oh!

scwhittle commented Aug 16, 2021

Uh oh!

scwhittle commented Aug 16, 2021

Uh oh!

lukecwik commented Aug 16, 2021

Uh oh!

lukecwik commented Aug 16, 2021

Uh oh!

scwhittle commented Aug 18, 2021

Uh oh!

chamikaramj commented Aug 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

scwhittle commented Aug 10, 2021 •

edited

Loading

`ValidatesRunner` compliance status (on master branch)