danny0405 commented on code in PR #18776:
URL: https://github.com/apache/hudi/pull/18776#discussion_r4060056526
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/io/TestHoodieCreateHandle.java:
##########
@@ -392,6 +393,40 @@ protected HoodieFileWriter initializeFileWriter() throws
IOException {
assertDoesNotThrow(createHandle::close);
}
+ @Test
+ void testFileWriterClosedWhenDoWriteFails() throws Exception {
+ HoodieWriteConfig failOnWriteConfig = HoodieWriteConfig.newBuilder()
+ .withProps(writeConfig.getProps())
+ .withWriteIgnoreFailed(false)
+ .build();
+ HoodieTable failOnWriteTable = new TestBaseHoodieTable(failOnWriteConfig,
getEngineContext(), metaClient);
+ CreateHandleWithFileWriterWriteFailure createHandle = new
CreateHandleWithFileWriterWriteFailure(
+ failOnWriteConfig, TEST_INSTANT_TIME, failOnWriteTable,
TEST_PARTITION_PATH, TEST_FILE_ID, taskContextSupplier);
+ HoodieRecord testRecord = dataGen.generateInserts(TEST_INSTANT_TIME,
1).get(0);
+
+ HoodieException exception = assertThrows(HoodieException.class, () ->
+ createHandle.doWrite(testRecord, TEST_SCHEMA, new TypedProperties()));
+
+ assertEquals("Simulated file writer write failure",
exception.getMessage());
+ assertNull(createHandle.fileWriter);
+ assertTrue(createHandle.isClosed());
+ assertDoesNotThrow(createHandle::close);
+ }
+
+ private static class CreateHandleWithFileWriterWriteFailure extends
HoodieCreateHandle<Object, Object, Object, Object> {
Review Comment:
Removed the duplicate local class. Both tests now use the shared
`CreateHandleWithFileWriterWriteFailure` member class.
##########
hudi-common/src/main/java/org/apache/hudi/common/bootstrap/index/hfile/HFileBootstrapIndexWriter.java:
##########
@@ -174,28 +176,60 @@ private void commit() {
* Close Writer Handles.
*/
public void close() {
- try {
- if (!closed) {
- indexByPartitionWriter.close();
- indexByFileIdWriter.close();
- closed = true;
- }
- } catch (IOException ioe) {
- throw new HoodieIOException(ioe.getMessage(), ioe);
+ if (closed) {
+ return;
+ }
+ Exception failure = closeHFileWriter(indexByPartitionWriter, null);
+ failure = closeHFileWriter(indexByFileIdWriter, failure);
+ indexByPartitionWriter = null;
+ indexByFileIdWriter = null;
+ closed = true;
+ if (failure != null) {
+ throw new HoodieException(failure.getMessage(), failure);
Review Comment:
Keeping the general `HoodieException` here for simplicity. I checked the
in-repository bootstrap writer callers and found no handling that depends on
the specific `HoodieIOException` subtype; the operation still fails with the
original exception retained as its cause.
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/parquet/io/TestHoodieParquetBinaryCopyBaseSchemaEvolution.java:
##########
@@ -290,6 +299,62 @@ public void
testSchemaEvolutionEnabled_AllowsLegacyConversion() throws Exception
assertEquals(true, legacyConversionAttempted, "Legacy conversion should be
attempted when schema evolution is enabled");
}
+ @Test
+ public void
testCloseParquetFileWriterQuietlyIgnoresWriterWithoutCloseMethod() {
Review Comment:
Removed the private-method reflection tests and the Method/Field helpers.
Close-lifecycle coverage now lives in `TestHoodieParquetBinaryCopyLifecycle`,
using package-private `@VisibleForTesting` accessors and the public `close()`
path. The tests account for whether the actual Parquet version implements
`AutoCloseable`.
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/io/TestHoodieAppendHandle.java:
##########
@@ -87,6 +100,38 @@ private void mockMethodsNeededByConstructor() {
when(mockHoodieTable.getMetaClient()).thenReturn(metaClient);
}
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ void testFailedFlushClosesWriterAndPreventsAnotherFlush(boolean closeFails)
throws IOException {
+ writeConfig = HoodieWriteConfig.newBuilder()
+ .withProps(writeConfig.getProps())
+ .withWriteTableVersion(HoodieTableVersion.SIX.versionCode())
+ .build();
+ when(mockHoodieTable.getStorage()).thenReturn(metaClient.getStorage());
+ HoodieInlineLogAppendHandle<Object, Object, Object, Object> handle = spy(
+ new HoodieInlineLogAppendHandle<>(writeConfig, TEST_INSTANT_TIME,
mockHoodieTable,
+ TEST_PARTITION_PATH, TEST_FILE_ID, taskContextSupplier));
+ HoodieLogFormat.Writer writer = mock(HoodieLogFormat.Writer.class);
+ handle.writer = writer;
+ handle.recordItr = Collections.emptyIterator();
+ handle.recordList.add(mock(HoodieRecord.class));
Review Comment:
Removed the unused buffered-record setup and its now-unused import.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
