HADOOP-19569. S3A: stream write/close fails badly once FS is closed #7700
+2,080
−1,129
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
steveloughran
force-pushed
the
s3/HADOOP-19569-stream-write-fs-close
branch
from
cf32b54 to
fea3e89
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR addresses HADOOP-19569 by enhancing the S3A filesystem’s behavior when closed, ensuring that write/close operations fail gracefully, and improves internal service registration and executor shutdown behavior. Key changes include enforcing FS state checks via added checkRunning() calls, refactoring helper and callback methods (e.g. renaming getWriteOperationHelper to createWriteOperationHelperWithinActiveSpan and replacing direct FS calls with getStore() invocations), and cleaning up legacy utilities such as MultipartUtils.
Reviewed Changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/MockS3AFileSystem.java | Adjusted WriteOperationHelper construction with updated callback parameters and method renaming. |
| hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AMiscOperations.java hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AClientSideEncryption.java |
Updated test calls to use getStore() for invoking putObjectDirect. |
| hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/S3AStoreImpl.java hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AStore.java |
Inserted checkRunning() calls across API methods and updated service lookup/register methods. |
| hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/WriteOperationHelper.java | Refactored to use a new “callbacks” field and improved null validations with requireNonNull. |
| hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java | Exposed getStore() publicly and updated multipart upload handling to leverage the new MultipartIOService. |
| hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestBlockingThreadPoolExecutorService.java hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/SemaphoredDelegatingExecutor.java hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/BlockingThreadPoolExecutorService.java |
Enhanced executor service tests and added rejection-handling logic to ensure proper shutdown on task rejection. |
Comments suppressed due to low confidence (3)
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/WriteOperationHelper.java:127
- [nitpick] Update the Javadoc in WriteOperationHelper to reflect the change from using the variable name 'writeOperationHelperCallbacks' to 'callbacks', ensuring that the documentation clearly explains its role and usage.
private final WriteOperationHelperCallbacks callbacks;
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AStore.java:290
- The completeMultipartUpload method has been removed from the S3AStore interface. Ensure that all external consumers have been updated accordingly or consider adding a deprecation warning to ease the transition.
/* Removed completeMultipartUpload method */
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java:1357
- [nitpick] Making the getStore() method public exposes internal implementation details. Confirm that this exposure is intended, or consider providing a more restricted accessor if external access is not required.
public S3AStore getStore() {
| slower than enqueueing. */ | ||
| final BlockingQueue<Runnable> workQueue = | ||
| new LinkedBlockingQueue<>(waitingTasks + activeTasks); | ||
| final InnerExecutorRejection rejection = new InnerExecutorRejection(); |
There was a problem hiding this comment.
[nitpick] The InnerExecutorRejection handler now shuts down the service upon rejection. Consider enhancing the error handling logic or adding more detailed documentation to explain the shutdown behavior in case of task rejection.
| private static final Integer SOME_VALUE = 1337; | ||
|
|
||
| private static BlockingThreadPoolExecutorService tpe; | ||
| private BlockingThreadPoolExecutorService tpe; |
There was a problem hiding this comment.
[nitpick] Since the thread pool executor is now an instance variable with setup/teardown methods, ensure that each test properly initializes and destroys the executor to avoid interference between tests.
steveloughran
force-pushed
the
s3/HADOOP-19569-stream-write-fs-close
branch
from
c347def to
c8cdbfa
Compare
steveloughran
force-pushed
the
s3/HADOOP-19569-stream-write-fs-close
branch
2 times, most recently
from
1dee57d to
b66a4e9
Compare
steveloughran
force-pushed
the
s3/HADOOP-19569-stream-write-fs-close
branch
from
b66a4e9 to
e90a8af
Compare
steveloughran
force-pushed
the
s3/HADOOP-19569-stream-write-fs-close
branch
from
e90a8af to
6fceeb3
Compare
steveloughran
force-pushed
the
s3/HADOOP-19569-stream-write-fs-close
branch
from
6fceeb3 to
1038637
Compare
steveloughran
force-pushed
the
s3/HADOOP-19569-stream-write-fs-close
branch
from
a20587d to
4a80578
Compare
steveloughran
force-pushed
the
s3/HADOOP-19569-stream-write-fs-close
branch
from
4a80578 to
b7fc4d2
Compare
All upload operations are in MultipartIO service, which has been renamed and move to package org.apache.hadoop.fs.s3a.impl.write to match. For completeness deletion should also go into this class or an adjacent one on deletion. Pulled out multipart IO such that there are no back references from it to S3AStore -the final change is to define a store statistics class which it and other things can use to update stats. Executors in hadoop-common to - pick up shutdown of inner executor and shut themselves down. - semaphore executor to decrement counters in this process so that queue state is updated - semaphored delegating executor unit test in common This stops callers being able to submit work when the inner executor has shut down. WriteOperationHelper * make all calls through its callback interface, rather than given a ref to S3AFS. * Move WriteOperationHelper callbacks to S3Store layer, Multipart IO operations * move nearly all Multpart IO operationss out of s3afs and into a new mulitpart service interface and impl * Multipart service retrieved and invoked as appropriate * StoreImpl stores a map of ServiceName -> service. with a lookupService() method in S3AStore interface, it's possible to retrieve services through the API just by knowing their name and type * registering all current services this way StoreImpl to IllegalStateException on method invocation whene the service isn't running. Some methods are kept open as they do seem needed.
steveloughran
force-pushed
the
s3/HADOOP-19569-stream-write-fs-close
branch
from
b7fc4d2 to
2290412
Compare
Contributor
Author
|
rebased. Tested against rustfs on localhost out of curiousity, so now I know what to say if someone asks if it can be used: "not if you need consistent directory listings"
Looking into the docs https://deepwiki.com/rustfs/rustfs/5.5-concurrency-management-and-caching#cache-coherence-and-invalidation comes up with "Invalidation is performed asynchronously to avoid blocking the write path." This is not as bad as AWS S3 used to be where a 404 was cached after a failing HEAD request, and listings took time to even find a newly created object. Conclusion: listings can see recently deleted files.
ListMultipartUploads operation sometimes returns an empty list even when MPUs are in progress, sometimes returns values when expected to be empty. all of these are failures saying
trivial and not unusual with third party stores. Overall, bit of a mess at least as far as testing goes. I would avoid using anywhere you require case sensitivitiy or list consistency, and would want to spend more time exploring the multipart test failures to be confident you can upload objects in sizes of many GB. That listing inconsistency brings back bad memories of the old S3, when hive and spark queries could miss data. I would absolutely avoid using any of the s3a committers against it without understanding what is up with MPU listings. |
Contributor
Author
|
@ahmarsuhail assuming you have time this week, can you take a quick look? |
|
💔 -1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
HADOOP-19569.
Executors in hadoop-common to
S3A code
This is complex.
TODO:
How was this patch tested?
New ITests which close the FS while simple and multipart writes are in progress.
S3 london.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?