Skip to content

HADOOP-19557 S3A: S3ABlockOutputStream to never log/reject hflush() calls #7662

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

Merged
steveloughran merged 1 commit into from
Apr 30, 2025
+6 −5
Merged

HADOOP-19557 S3A: S3ABlockOutputStream to never log/reject hflush() calls #7662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3ABlockOutputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,7 +829,8 @@ public boolean hasCapability(String capability) {
@Override
public void hflush() throws IOException {
statistics.hflushInvoked();
handleSyncableInvocation();
// do not reject these, but downgrade to a no-oop
LOG.debug("Hflush invoked");
Copy link
Contributor

@ahmarsuhail ahmarsuhail Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@steveloughran is parquet the only reader calling hflush? think this changes behaviour for everyone.. is this something we need to care about?

Copy link
Contributor Author

@steveloughran steveloughran Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

@Override
Expand All @@ -839,7 +840,7 @@ public void hsync() throws IOException {
}

/**
* Shared processing of Syncable operation reporting/downgrade.
* Processing of Syncable operation reporting/downgrade.
* @throws UnsupportedOperationException if required.
*/
private void handleSyncableInvocation() {
Expand Down
6 changes: 3 additions & 3 deletions hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/TestS3ABlockOutputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,21 +148,21 @@ public void testCallingCloseAfterCallingAbort() throws Exception {

/**
* Unless configured to downgrade, the stream will raise exceptions on
* Syncable API calls.
* Syncable.hsync() API calls.
*/
@Test
public void testSyncableUnsupported() throws Exception {
final S3ABlockOutputStream.BlockOutputStreamBuilder
builder = mockS3ABuilder();
builder.withDowngradeSyncableExceptions(false);
stream = spy(new S3ABlockOutputStream(builder));
intercept(UnsupportedOperationException.class, () -> stream.hflush());
stream.hflush();
intercept(UnsupportedOperationException.class, () -> stream.hsync());
}

/**
* When configured to downgrade, the stream downgrades on
* Syncable API calls.
* Syncable.hsync() API calls.
*/
@Test
public void testSyncableDowngrade() throws Exception {
Expand Down