Fix file fingerprint to use atomic get content hash of uploaded file #16597

tcrain · 2022-11-30T03:11:24Z

What changes are proposed in this pull request?

Currently when complete is called on a file in Alluxio, a fingerprint of the file will be created by performing a GetStauts on the file on the UFS. If due to a concurrent write, the state of the file is different than what was written through Alluxio, the fingerprint will not actually match the content of the file in Alluxio. If this happens the state of the file in Alluxio will always be out of sync with the UFS, and the file will never be updated to the most recent version.
This is because metadata sync uses the fingerprint to see if the file needs synchronization, and if the fingerprint does not match the file in Alluxio there will be inconsistencies.

This PR fixes this by having the contentHash field of the fingerprint be computed while the file is actually written on the UFS. For object stores, this means the hash is taken from the result of the call to PutObject. Unfortunately HDFS does not have a similar interface, so the content hash is taken just after the output stream is closed to complete the write. There could be a small chance that someone changes the file in this window between the two operations.

alluxio-bot · 2022-11-30T03:11:29Z

Automated checks report:

Commits associated with Github account: PASS
PR title follows the conventions: FAIL
- The title of the PR does not pass all the checks. Please fix the following issues:
  - First word of title ("Atomic") is not an imperative verb. Please use one of the valid words

Some checks failed. Please fix the reported issues and reply 'alluxio-bot, check this please' to re-run checks.

lucyge2022 · 2022-11-30T18:35:28Z

core/server/master/src/main/java/alluxio/master/file/DefaultFileSystemMaster.java

@@ -1699,7 +1699,8 @@ void completeFileInternal(RpcContext rpcContext, LockedInodePath inodePath,
      try (CloseableResource<UnderFileSystem> ufsResource = resolution.acquireUfsResource()) {
        UnderFileSystem ufs = ufsResource.get();
        if (ufsStatus == null) {
-          ufsFingerprint = ufs.getParsedFingerprint(ufsPath).serialize();
+          ufsFingerprint = ufs.getParsedFingerprint(ufsPath, context.getOptions().hasContentHash()
+              ? context.getOptions().getContentHash() : null).serialize();


will we have a case where input contentHash is diff than the ufs status contenthash, meaning at time of completefile ufs has changed to some other content, and we are updating fileinode with our version of contenthash but with other field uptodate with ufs like owner ? or at least we could reload if we find contenthash mismatch later

What I am going for here is to have the data in Alluxio match the fingerprint created. The first part is to match the contents of the file which is done here. The next part will be to match the metadata (this PR is still in progress). I might be able to remove the UFS access here all together.

I am thinking that even if the data in Alluxio is out of sync with the UFS it is fine here, as later synchronizations will happen based on the metadata sync policy. The main thing that matters is that the fingerprint matches the actual data so that if there is an updated file on the UFS it will be synced whenever the next sync happens based on the policy.

So after internal discussion, only the content hash of the file will be kept consistent.

This is due to other metadata in the fingerprint being computed in so many different ways, with all kinds of strange behaviors that clients might rely on, it was better not to mess with this as it could break things.

github-actions · 2023-01-09T20:09:08Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions.

LuQQiu · 2023-01-09T20:26:11Z

@lucyge2022 @tcrain PTAL too see if the changes are still valid? Please help work on the PR or close it otherwise

…mmit

alluxio-bot · 2023-01-25T03:10:51Z

Automated checks report:

Commits associated with Github account: PASS
PR title follows the conventions: PASS

All checks passed!

LuQQiu · 2023-02-14T02:27:52Z

@tcrain is this PR ready for another round of review?

tcrain · 2023-02-14T23:16:30Z

@tcrain is this PR ready for another round of review?

@LuQQiu If you review, I'll also make another PR that will help with #16833

LuQQiu

Overall LGTM, left two comments about the code arrangement

LuQQiu · 2023-02-15T18:10:39Z

core/common/src/main/java/alluxio/underfs/UnderFileSystemOutputStream.java

+ * content written when the stream is closed. The content hash will then be used as part of
+ * the metadata fingerprint when the file is completed on the Alluxio master.
+ */
+public interface UnderFileSystemOutputStream {


Can we rename it to more concrete?

LuQQiu · 2023-02-15T18:12:48Z

underfs/obs/src/main/java/alluxio/underfs/obs/OBSOutputStream.java

@@ -42,7 +44,7 @@
 * local disk and copied as a complete file when the {@link #close()} method is called.
 */
 @NotThreadSafe
-public final class OBSOutputStream extends OutputStream {
+public final class OBSOutputStream extends OutputStream implements UnderFileSystemOutputStream {


Can you try to move the contentHash function into OutputStream and have default implementation, the logics seem similar

I couldn't figure out a good way to do this, since different classes extend different types out output streams I think I have to use an interface. Unless you can think of a better way?

LuQQiu

LGTM, left one minor comment, please fix the test failures

LuQQiu · 2023-02-15T22:02:18Z

core/common/src/main/java/alluxio/underfs/ContentHashableOutputStream.java

@@ -20,7 +20,7 @@
 * content written when the stream is closed. The content hash will then be used as part of
 * the metadata fingerprint when the file is completed on the Alluxio master.
 */
-public interface UnderFileSystemOutputStream {
+public interface ContentHashableOutputStream {


Without OutputStream? similar to Seekable?

LuQQiu · 2023-02-16T00:20:17Z

alluxio-bot, merge this please

Currently when complete is called on a file in Alluxio, a fingerprint of the file will be created by performing a GetStauts on the file on the UFS. If due to a concurrent write, the state of the file is different than what was written through Alluxio, the fingerprint will not actually match the content of the file in Alluxio. If this happens the state of the file in Alluxio will always be out of sync with the UFS, and the file will never be updated to the most recent version. This is because metadata sync uses the fingerprint to see if the file needs synchronization, and if the fingerprint does not match the file in Alluxio there will be inconsistencies. This PR fixes this by having the contentHash field of the fingerprint be computed while the file is actually written on the UFS. For object stores, this means the hash is taken from the result of the call to PutObject. Unfortunately HDFS does not have a similar interface, so the content hash is taken just after the output stream is closed to complete the write. There could be a small chance that someone changes the file in this window between the two operations. pr-link: Alluxio#16597 change-id: cid-64723be309bdb14b05613864af3b6a1bb30cba6d

Cherry-pick of existing commit. orig-pr: #16597 orig-commit: 00da77c orig-commit-author: Tyler Crain <tycrain@gmail.com> pr-link: #17361 change-id: cid-64723be309bdb14b05613864af3b6a1bb30cba6d

… get content hash of uploaded file Currently when complete is called on a file in Alluxio, a fingerprint of the file will be created by performing a GetStauts on the file on the UFS. If due to a concurrent write, the state of the file is different than what was written through Alluxio, the fingerprint will not actually match the content of the file in Alluxio. If this happens the state of the file in Alluxio will always be out of sync with the UFS, and the file will never be updated to the most recent version. This is because metadata sync uses the fingerprint to see if the file needs synchronization, and if the fingerprint does not match the file in Alluxio there will be inconsistencies. This PR fixes this by having the contentHash field of the fingerprint be computed while the file is actually written on the UFS. For object stores, this means the hash is taken from the result of the call to PutObject. Unfortunately HDFS does not have a similar interface, so the content hash is taken just after the output stream is closed to complete the write. There could be a small chance that someone changes the file in this window between the two operations. pr-link: Alluxio#16597 change-id: cid-64723be309bdb14b05613864af3b6a1bb30cba6d

Currently when complete is called on a file in Alluxio, a fingerprint of the file will be created by performing a GetStauts on the file on the UFS. If due to a concurrent write, the state of the file is different than what was written through Alluxio, the fingerprint will not actually match the content of the file in Alluxio. If this happens the state of the file in Alluxio will always be out of sync with the UFS, and the file will never be updated to the most recent version. This is because metadata sync uses the fingerprint to see if the file needs synchronization, and if the fingerprint does not match the file in Alluxio there will be inconsistencies. This PR fixes this by having the contentHash field of the fingerprint be computed while the file is actually written on the UFS. For object stores, this means the hash is taken from the result of the call to PutObject. Unfortunately HDFS does not have a similar interface, so the content hash is taken just after the output stream is closed to complete the write. There could be a small chance that someone changes the file in this window between the two operations. pr-link: Alluxio#16597 change-id: cid-64723be309bdb14b05613864af3b6a1bb30cba6d

tcrain added 3 commits November 29, 2022 18:39

atomic get content hash of uploaded file

f6fa2c7

update message stream response

97552d8

update interface

97fdd60

lucyge2022 reviewed Nov 30, 2022

View reviewed changes

tcrain added 3 commits December 1, 2022 13:02

Merge branch 'master' into fingerprint-on-commit

290c39e

add fingerprint

e01e35d

add lisc header

f08736a

github-actions bot added the stale The PR/Issue does not have recent activities and will be closed automatically label Jan 9, 2023

github-actions bot removed the stale The PR/Issue does not have recent activities and will be closed automatically label Jan 10, 2023

jja725 self-requested a review January 12, 2023 00:17

tcrain added 6 commits January 20, 2023 15:46

Merge remote-tracking branch 'upstream/master' into fingerprint-on-co…

e5d9670

…mmit

update fingerprint test

889f091

Merge branch 'master' into fingerprint-on-commit

4f62262

update tests

6cbf301

fix tests

e95a3de

revert change to fsrename test

af541fe

tcrain changed the title ~~Atomic get content hash of uploaded file~~ Fix file fingerprint to use atomic get content hash of uploaded file Jan 25, 2023

alluxio-bot added the API Change Changes covering public API label Jan 25, 2023

alluxio-bot assigned tcrain Jan 25, 2023

LuQQiu mentioned this pull request Feb 14, 2023

Getting Error reading from alluxio://<host>:19998/<path_to_parquet_file> at position xxxxxxx in presto SQL #16833

Open

tcrain added 3 commits February 14, 2023 11:47

merge with master

4d31fa5

fix tests

3ead2e5

fix checkstyle

4205b02

Merge branch 'master' into fingerprint-on-commit

56cb0c2

LuQQiu reviewed Feb 15, 2023

View reviewed changes

rename field

98260ad

LuQQiu approved these changes Feb 15, 2023

View reviewed changes

tcrain added 2 commits February 15, 2023 14:03

fix checkstyle

9972cb4

update name

228c27e

alluxio-bot merged commit 00da77c into Alluxio:master Feb 16, 2023

Xenorith added the area-master Alluxio master label Feb 28, 2023

jja725 mentioned this pull request May 1, 2023

Add content hash to Dora #17361

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix file fingerprint to use atomic get content hash of uploaded file #16597

Fix file fingerprint to use atomic get content hash of uploaded file #16597

tcrain commented Nov 30, 2022 •

edited

Loading

alluxio-bot commented Nov 30, 2022

lucyge2022 Nov 30, 2022

tcrain Dec 1, 2022

tcrain Jan 25, 2023

github-actions bot commented Jan 9, 2023

LuQQiu commented Jan 9, 2023

alluxio-bot commented Jan 25, 2023

LuQQiu commented Feb 14, 2023

tcrain commented Feb 14, 2023

LuQQiu left a comment

LuQQiu Feb 15, 2023

LuQQiu Feb 15, 2023

tcrain Feb 15, 2023

LuQQiu left a comment

LuQQiu Feb 15, 2023

LuQQiu commented Feb 16, 2023

Fix file fingerprint to use atomic get content hash of uploaded file #16597

Fix file fingerprint to use atomic get content hash of uploaded file #16597

Conversation

tcrain commented Nov 30, 2022 • edited Loading

What changes are proposed in this pull request?

alluxio-bot commented Nov 30, 2022

lucyge2022 Nov 30, 2022

Choose a reason for hiding this comment

tcrain Dec 1, 2022

Choose a reason for hiding this comment

tcrain Jan 25, 2023

Choose a reason for hiding this comment

github-actions bot commented Jan 9, 2023

LuQQiu commented Jan 9, 2023

alluxio-bot commented Jan 25, 2023

LuQQiu commented Feb 14, 2023

tcrain commented Feb 14, 2023

LuQQiu left a comment

Choose a reason for hiding this comment

LuQQiu Feb 15, 2023

Choose a reason for hiding this comment

LuQQiu Feb 15, 2023

Choose a reason for hiding this comment

tcrain Feb 15, 2023

Choose a reason for hiding this comment

LuQQiu left a comment

Choose a reason for hiding this comment

LuQQiu Feb 15, 2023

Choose a reason for hiding this comment

LuQQiu commented Feb 16, 2023

tcrain commented Nov 30, 2022 •

edited

Loading