HADOOP-17981 Support etag-assisted renames in FileOutputCommitter #3597

steveloughran · 2021-10-28T15:48:28Z

Contains MAPREDUCE-7367. #2971
Parallelize file moves in FileOutputCommitter v1 job commit

Contributed by Rajesh Balamohan

For code changes:

Does the title or this PR starts with the corresponding JIRA issue id (e.g. 'HADOOP-17799. Your PR title ...')?
Object storage: have the integration tests been executed and the endpoint declared according to the connector-specific documentation?

...he/hadoop/mapreduce/lib/output/committer/manifest/impl/StoreOperationsThroughFileSystem.java

…commit Contributed by Rajesh Balamohan Change-Id: I8d2cedb0ba8b6aba2f1d0f814f52d77b156f1206

Selective cherrypick of MAPREDUCE-7341 files; move etag tests into hadoop-azure. RawLocalFS implements the API; checksum fs does not Change-Id: Ic68337742de6aefd067eda14313fd07088c73ac7

adds options vararg similar to rename/3; allows caller to declare it is confident the dest path doesn't exist, so no need to check for overwrites or similar. test on local fs verifies that renaming on a filesystem without the new interface matches that of the new behaviour, committer test alternates between raw/local fs on parameterized runs. minor reorg of the new tests in that class Change-Id: Ic31d52c9a95b768e0a97583e906c20c53b120194

sidseth · 2021-11-02T04:56:11Z

...ce-client-core/src/main/java/org/apache/hadoop/mapreduce/lib/output/FileOutputCommitter.java

+      Path finalOutput = getOutputPath();
+      FileSystem fs = finalOutput.getFileSystem(context.getConfiguration());
+      // created resilient commit helper bonded to the destination FS/path
+      resilientCommitHelper = new ResilientCommitByRenameHelper(fs);


This mechanism becomes very FileSystem specific. Implemented by Azure right now.
Other users of rename will not see the benefits without changing interfaces, which in turn requires shimming etc.

Would it be better for AzureFileSystem rename itself to add a config parameter which can lookup the src etag (at the cost of a performance hit for consistency), so that downstream components / any users of the rename operation can benefit from this change without having to change interfaces. Also, if the performance penalty is a big problem - Abfs could create very short-lived caches for FileStatus objects, and handle errors on discrepancies with the cached copy.
Essentially - don't force usage of the new interface to get the benefits.

Side note: The fs.getStatus within ResilientCommitByRenameHelper for FileSystems where this new functionality is not supported will lead to a performance penalty for the other FileSystems (performing a getFileStatus on src).

mukund-thakur

Looks good overall. Pending ABFS side changes.

...n-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/ResilientCommitByRename.java

steveloughran · 2021-11-02T10:27:53Z

This mechanism becomes very FileSystem specific. Implemented by Azure right now.

I agree, which is why the API is restricted for its uses to mr-client-core only.
as abfs is the only one which needs it for correctness under load,
And I'm not worried about that specifity.
Can I point to how much of the hadoop fs api are hdfs-only -and they are public.

Other users of rename will not see the benefits without changing interfaces, which in turn requires shimming etc.

Please don't try and use this particular interface in Hive.

Would it be better for AzureFileSystem rename itself to add a config parameter which can lookup the src etag (at the cost of a performance hit for consistency), so that downstream components / any users of the rename operation can benefit from this change without having to change interfaces.

We are going straight from a listing (1 request/500 entries) to that rename. doing a HEAD first cuts the throughtput in half. so no.

Also, if the performance penalty is a big problem - Abfs could create very short-lived caches for FileStatus objects, and handle errors on discrepancies with the cached copy.

Possible but convoluted.

Essentially - don't force usage of the new interface to get the benefits.

I understand the interests of the hive team, but this fix is not the place to do a better API.

Briefly cacheing the source FS entries is something to consider though. Not this week.

What I could do with is some help getting #2735 in, then we can start on a public rename() builder API which will take a file status, as openFile does.

Side note: The fs.getStatus within ResilientCommitByRenameHelper for FileSystems where this new functionality is not supported will lead to a performance penalty for the other FileSystems (performing a getFileStatus on src).

There is an option to say "i know it is not there"; this skips the check. the committer passes this option down because it issues a delete call first.

FWIW the manifest committer will make that pre-rename commit optional, saving that IO request. I am curious as to how well that will work I went executed on well formed tables.

Change-Id: I65dd20e2e0d6af2d347c07b4a3e13ab58763c1ac

Change-Id: I79297a308bf558d188592cadbe60e2622435c6e8

steveloughran · 2021-11-02T13:18:56Z

@sidseth I discussed the idea of caching status etags with @mukund-thakur

considering that you'd need to remove entries from the cache on: create, rename, & delete, it would actually be a copy of org.apache.hadoop.fs.s3a.s3guard.LocalMetadataStore, as #3534 is removing all of s3guard, I'll consider that a WONTFIX. sorry.

steveloughran · 2021-11-02T18:20:05Z

see #3611 . i think this pr had got too complex and so have gone back to what I had originally done in the manifest committer. filesystem provides etags, application code recovers.

This is something which we can expose to the hive team, as all that will be added is a single stable extension to filestatus to get the etag.

sidseth · 2021-11-03T05:04:54Z

This mechanism becomes very FileSystem specific. Implemented by Azure right now.

I agree, which is why the API is restricted for its uses to mr-client-core only. as abfs is the only one which needs it for correctness under load, And I'm not worried about that specifity. Can I point to how much of the hadoop fs api are hdfs-only -and they are public.

Other users of rename will not see the benefits without changing interfaces, which in turn requires shimming etc.

Please don't try and use this particular interface in Hive.

Was referring to any potential usage - including Hive.

Would it be better for AzureFileSystem rename itself to add a config parameter which can lookup the src etag (at the cost of a performance hit for consistency), so that downstream components / any users of the rename operation can benefit from this change without having to change interfaces.

We are going straight from a listing (1 request/500 entries) to that rename. doing a HEAD first cuts the throughtput in half. so no.

In the scenario where this is encountered. Would not be the default behaviour, and limits the change to Abfs. Could also have the less consistent version which is not etag based, and responds only on failures. Again - limited to Abfs.

Also, if the performance penalty is a big problem - Abfs could create very short-lived caches for FileStatus objects, and handle errors on discrepancies with the cached copy.

Possible but convoluted.

Agree. Quite convoluted. Tossing in potential options - to avoid a new public API.

Essentially - don't force usage of the new interface to get the benefits.

I understand the interests of the hive team, but this fix is not the place to do a better API.

Briefly cacheing the source FS entries is something to consider though. Not this week.

What I could do with is some help getting #2735 in, then we can start on a public rename() builder API which will take a file status, as openFile does.

This particular change would be FSImpl agnostic, and potentially remove the need for the new interface here?

Side note: The fs.getStatus within ResilientCommitByRenameHelper for FileSystems where this new functionality is not supported will lead to a performance penalty for the other FileSystems (performing a getFileStatus on src).

There is an option to say "i know it is not there"; this skips the check. the committer passes this option down because it issues a delete call first.

EOD - this ends up being a new API (almost on the FileSystem), which is used by the committer first; then someone discovers it and decides to make use of it.

FWIW the manifest committer will make that pre-rename commit optional, saving that IO request. I am curious as to how well that will work I went executed on well formed tables.

steveloughran · 2021-11-03T14:14:04Z

EOD - this ends up being a new API (almost on the FileSystem), which is used by the committer first; then someone discovers it and decides to make use of it.

yes, and both hive and hbase are known to do that, often ending forcing hdfs only apis to get pulled up without concern for the other stores (see git history of FileSystem there). a builder rename would be a big job with

rename/3 public with tests and performant object store impls
default builder to delegate to that with same semantics
add options for etag etc on a store by store or cross store basis

steveloughran force-pushed the mr/HADOOP-17981-resilient-commit branch from 08e20d0 to ee6854c Compare October 29, 2021 09:40

mukund-thakur reviewed Oct 29, 2021

View reviewed changes

...he/hadoop/mapreduce/lib/output/committer/manifest/impl/StoreOperationsThroughFileSystem.java Outdated Show resolved Hide resolved

Rajesh Balamohan and others added 2 commits October 29, 2021 18:06

MAPREDUCE-7367. Parallelize file moves in FileOutputCommitter v1 job …

a85d778

…commit Contributed by Rajesh Balamohan Change-Id: I8d2cedb0ba8b6aba2f1d0f814f52d77b156f1206

HADOOP-17981. ResilientCommitByRename

f4950b3

Selective cherrypick of MAPREDUCE-7341 files; move etag tests into hadoop-azure. RawLocalFS implements the API; checksum fs does not Change-Id: Ic68337742de6aefd067eda14313fd07088c73ac7

steveloughran force-pushed the mr/HADOOP-17981-resilient-commit branch from ee6854c to f4950b3 Compare October 29, 2021 17:07

apache deleted a comment from hadoop-yetus Nov 1, 2021

sidseth reviewed Nov 2, 2021

View reviewed changes

mukund-thakur reviewed Nov 2, 2021

View reviewed changes

...n-project/hadoop-common/src/main/java/org/apache/hadoop/fs/impl/ResilientCommitByRename.java Outdated Show resolved Hide resolved

steveloughran added 2 commits November 2, 2021 11:51

HADOOP-17981. changes during a review with Mukund

658898b

Change-Id: I65dd20e2e0d6af2d347c07b4a3e13ab58763c1ac

HADOOP-17981. add a test I'd forgotten to commit

72bc6fd

Change-Id: I79297a308bf558d188592cadbe60e2622435c6e8

steveloughran mentioned this pull request Nov 2, 2021

HADOOP-17981. resilient commit through etag validation #3611

Closed

4 tasks

apache deleted a comment from hadoop-yetus Nov 3, 2021

steveloughran closed this Nov 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

HADOOP-17981 Support etag-assisted renames in FileOutputCommitter #3597

HADOOP-17981 Support etag-assisted renames in FileOutputCommitter #3597

Uh oh!

steveloughran commented Oct 28, 2021 •

edited

Loading

Uh oh!

Uh oh!

sidseth Nov 2, 2021

Uh oh!

mukund-thakur left a comment

Uh oh!

Uh oh!

steveloughran commented Nov 2, 2021

Uh oh!

steveloughran commented Nov 2, 2021

Uh oh!

steveloughran commented Nov 2, 2021

Uh oh!

sidseth commented Nov 3, 2021

Uh oh!

steveloughran commented Nov 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

HADOOP-17981 Support etag-assisted renames in FileOutputCommitter #3597

HADOOP-17981 Support etag-assisted renames in FileOutputCommitter #3597

Uh oh!

Conversation

steveloughran commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

For code changes:

Uh oh!

Uh oh!

sidseth Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

mukund-thakur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

steveloughran commented Nov 2, 2021

Uh oh!

steveloughran commented Nov 2, 2021

Uh oh!

steveloughran commented Nov 2, 2021

Uh oh!

sidseth commented Nov 3, 2021

Uh oh!

steveloughran commented Nov 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

steveloughran commented Oct 28, 2021 •

edited

Loading