HADOOP-19557 S3A: S3ABlockOutputStream to never log/reject hflush() calls #7662
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
…alls Calls to Syncable.hflush() on S3ABlockOutputStream instances are logged at debug and the statistics counter upgraded
|
🎊 +1 overall
This message was automatically generated. |
ahmarsuhail
reviewed
Apr 30, 2025
| statistics.hflushInvoked(); | ||
| handleSyncableInvocation(); | ||
| // do not reject these, but downgrade to a no-oop | ||
| LOG.debug("Hflush invoked"); |
Contributor
There was a problem hiding this comment.
@steveloughran is parquet the only reader calling hflush? think this changes behaviour for everyone.. is this something we need to care about?
Contributor
Author
There was a problem hiding this comment.
look at the fs spec. we say "don't use the api and highlight the inconsistent outcomes"
The semantics of hflush say "visible to all" but no persistence, so it's not changing any durability semantics. Are we changing the visibility? we're certainly not meeting them.
I remember having a long talk with others about hflush, as in "what does it do?" -the answer is "nothing you can rely on".
when exceptions are downgraded (default) all that happens is the log message is removed, so reducing confusion.
when exceptions are rejected, the failure goes away. The one I want to fail here is hsync(), and at holds. AFAIK nobody runs with that flag on except for some of our test setups.
Syncable.hflush()
Flush out the data in client's user buffer. After the return of
this call, new readers will see the data. The hflush() operation
does not contain any guarantees as to the durability of the data. only
its visibility.
Thus implementations may cache the written data in memory
—visible to all, but not yet persisted.
steveloughran
added a commit
to steveloughran/hadoop
that referenced
this pull request
Apr 30, 2025
…alls (apache#7662) S3A output streams no longer logs warnings on use of hflush() or, if fs.s3a.downgrade.syncable.exceptions = false, raises an UnsupportedOperationException . hsync() is still reported with a warning or rejected. That method is absolutely unsupported when writing to S3. Contributed by Steve Loughran
4 tasks
steveloughran
added a commit
that referenced
this pull request
May 1, 2025
…calls (#7662) S3A output streams no longer logs warnings on use of hflush() or, if fs.s3a.downgrade.syncable.exceptions = false, raises an UnsupportedOperationException . hsync() is still reported with a warning or rejected. That method is absolutely unsupported when writing to S3. Contributed by Steve Loughran
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Calls to Syncable.hflush() on S3ABlockOutputStream instances are logged at debug and the statistics counter upgraded
How was this patch tested?
-Dparallel-tests -DtestsThreadCount=9For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?