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.

Sign up for GitHub

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

Jump to bottom

Catch `RemoteFileChangedException` inside of `S3DynamoDBLogStore::read` #1712

Closed

scottsand-db wants to merge 12 commits into delta-io:master from scottsand-db:s3_ddb_logstore_catch_remote_file_changed_exception

Collaborator

scottsand-db commented Apr 21, 2023 •

edited

Loading

Description

In S3, if N.json is over-written while an input stream is open on it, then the ETag will change and a RemoteFileChangedException will be thrown.

This PR adds logic to retry reading that N.json file, at the exact same line that the error occurred at.

This assumes and requires that the contents of N.json have been overwritten with the same identical content!

As an important implementation highlight: so, if we are at index 25 (thus, the last successfully read index is 24), and we try to call .next() on the read iterator, and an RemoteFileChangedException is thrown, we will re-generate the read iterator, skip all the way to index 25, and try reading it again.

How was this patch tested?

New UTs.

Does this PR introduce any user-facing changes?

No.

scottsand-db added 2 commits

April 21, 2023 15:48


          Create and use RetryableCloseableIterator class

e51c3d9


          Create RetryableCloseableIteratorSuite.scala

89ad977

scottsand-db self-assigned this

scottsand-db commented

View reviewed changes

storage-s3-dynamodb/src/main/java/io/delta/storage/RetryableCloseableIterator.java Outdated Show resolved Hide resolved

Contributor

tdas commented Apr 24, 2023

Specify in the description that the content of the overwritten file is expected to be exactly same. Without that this entire thing does not make sense at all.

tdas reviewed

View reviewed changes

storage-s3-dynamodb/src/main/java/io/delta/storage/RetryableCloseableIterator.java Outdated Show resolved Hide resolved

tdas reviewed

View reviewed changes

storage-s3-dynamodb/src/main/java/io/delta/storage/RetryableCloseableIterator.java Outdated Show resolved Hide resolved

tdas reviewed

View reviewed changes

storage-s3-dynamodb/src/main/java/io/delta/storage/RetryableCloseableIterator.java Outdated Show resolved Hide resolved

scottsand-db added 2 commits

April 24, 2023 09:57


          close the current iter after failure

533bdb5


          Update RetryableCloseableIterator.java

7ebc93f

- update variable names
- update class docs

scottsand-db requested review from tdas, zsxwing and vkorukanti

April 24, 2023 17:20

zsxwing reviewed

View reviewed changes

build.sbt Show resolved Hide resolved

storage-s3-dynamodb/src/main/java/io/delta/storage/RetryableCloseableIterator.java Outdated Show resolved Hide resolved

storage-s3-dynamodb/src/main/java/io/delta/storage/RetryableCloseableIterator.java Outdated Show resolved Hide resolved

storage-s3-dynamodb/src/main/java/io/delta/storage/RetryableCloseableIterator.java Outdated Show resolved Hide resolved

storage-s3-dynamodb/src/main/java/io/delta/storage/RetryableCloseableIterator.java Outdated Show resolved Hide resolved

storage-s3-dynamodb/src/main/java/io/delta/storage/S3DynamoDBLogStore.java Outdated Show resolved Hide resolved

storage-s3-dynamodb/src/main/java/io/delta/storage/RetryableCloseableIterator.java Outdated Show resolved Hide resolved

zsxwing reviewed

View reviewed changes

storage-s3-dynamodb/src/main/java/io/delta/storage/RetryableCloseableIterator.java Outdated Show resolved Hide resolved

scottsand-db added 4 commits

April 24, 2023 17:09


          Handle exceptions even while retrying; TODO unit test

e9450dc


          Refactor and clean up tests

12e1dae


          Add a conf to set the max number of retries

1e30ed8


          Add test for retried iter doesn't have enough data

de97330

scottsand-db requested a review from zsxwing

April 25, 2023 17:00


          update exception msg

c05d0c5

zsxwing reviewed

View reviewed changes

Member

zsxwing left a comment

Mostly style comments.

One improvement we may consider in the future is that we can calculate the md5 for the file content we have seen. When replaying the iterator, we can calculate the md5 for the new content and compare it with the existing value we have. This would allow us to detect really data loss issues. Of cause, this can only detect unexpected file overwriting when reading a json file. If the unexpected file overwriting happens when we are not reading the file, there is nothing we can do.

storage-s3-dynamodb/src/main/java/io/delta/storage/RetryableCloseableIterator.java Outdated Show resolved Hide resolved

storage-s3-dynamodb/src/main/java/io/delta/storage/RetryableCloseableIterator.java Outdated Show resolved Hide resolved

storage-s3-dynamodb/src/main/java/io/delta/storage/RetryableCloseableIterator.java Outdated Show resolved Hide resolved

storage-s3-dynamodb/src/main/java/io/delta/storage/RetryableCloseableIterator.java Outdated Show resolved Hide resolved

storage-s3-dynamodb/src/main/java/io/delta/storage/RetryableCloseableIterator.java Outdated Show resolved Hide resolved

storage-s3-dynamodb/src/main/java/io/delta/storage/RetryableCloseableIterator.java Show resolved Hide resolved

storage-s3-dynamodb/src/test/scala/io/delta/storage/RetryableCloseableIteratorSuite.scala Outdated Show resolved Hide resolved

storage-s3-dynamodb/src/test/scala/io/delta/storage/RetryableCloseableIteratorSuite.scala Outdated Show resolved Hide resolved

storage-s3-dynamodb/src/test/scala/io/delta/storage/RetryableCloseableIteratorSuite.scala Outdated Show resolved Hide resolved

storage-s3-dynamodb/src/main/java/io/delta/storage/RetryableCloseableIterator.java Show resolved Hide resolved

scottsand-db added 2 commits

April 27, 2023 09:27


          Handle FileAlreadyExistsException in fixDeltaLog

4c70285


          Clean up PR & respond to PR comments

0cf30e2

scottsand-db requested a review from zsxwing

April 27, 2023 17:26

zsxwing approved these changes

View reviewed changes

Member

zsxwing left a comment

RetryableCloseableIterator.java looks good to me.

Left only one comment about the FileAlreadyExistsException issue. You can fix it in this PR and update the PR description, or revert changes for FileAlreadyExistsException and fix it in a separate PR.

storage-s3-dynamodb/src/main/java/io/delta/storage/BaseExternalLogStore.java Outdated

                               fixDeltaLogPutCompleteDbEntry(entry);
                               LOG.info("fixed {}", entry.fileName);
                               return;
-                          } catch(Throwable e) {
+                          } catch (java.nio.file.FileAlreadyExistsException e) {

Member

zsxwing Apr 28, 2023

writeCopyTempFile calls copyFile, which has the same issue (a concurrent reader or writer copies the file before writeCopyTempFile is finished). Should we just change copyFile instead?

Collaborator Author

scottsand-db Apr 28, 2023

hm, so what would that change be?

copyFile does not throw FIleAlreadyExistsException? i.e. the assumption is that we will only ever copy T(N) -> N.json, (i.e. no other content will be written to N.json .... so if we do duplicate copies then that's fine, don't throw)
copyFile copies with overwrite=true?

Member

zsxwing Apr 28, 2023

copyFile does not throw FIleAlreadyExistsException?

This one

copyFile copies with overwrite=true?

Not this one because we want to avoid overwrite if possible


          Revert "Handle FileAlreadyExistsException in fixDeltaLog"

038c0b8

This reverts commit 4c70285.

zsxwing approved these changes

View reviewed changes

Member

zsxwing left a comment

LGTM

scottsand-db requested a review from zsxwing

April 28, 2023 17:19

scottsand-db closed this in

d7483ad

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet