hudi-agent commented on code in PR #20118:
URL: https://github.com/apache/hudi/pull/20118#discussion_r4127205232
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/io/cdc/TestHoodieNativeLogFormatWriter.java:
##########
@@ -208,6 +216,106 @@ public void
testSkipsDataFileFormatMetadataWhenColumnStatsDisabled() throws Exce
assertFalse(metadata.isPresent());
}
+ @ParameterizedTest
+ @CsvSource({
+ "io, false, false", "io, false, true",
+ "io, true, false", "io, true, true",
+ "runtime, true, false", "runtime, true, true",
Review Comment:
🤖 nit: the failureMode strings ("io", "runtime", "metadata", "error",
"none", "shared") are magic values whose meaning only becomes clear once you
read the method body. Consider a small enum or at least a comment near the
@CsvSource block explaining what each mode simulates.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/cdc/HoodieNativeLogFormatWriter.java:
##########
@@ -277,27 +278,41 @@ private void ensureAppendVersion() throws IOException {
}
private void closeFileWriters() throws IOException {
- if (dataFileWriter != null) {
- dataFileWriter.close();
- if (writeConfig.isMetadataColumnStatsIndexEnabled()) {
- try {
- lastDataFileFormatMetadata =
Option.ofNullable(dataFileWriter.getFileFormatMetadata());
- } catch (UnsupportedOperationException e) {
- // File-format metadata is an optional writer capability.
Unsupported formats can still produce valid
- // native log files; they simply do not contribute column stats for
this append.
+ try {
+ try {
+ if (dataFileWriter != null) {
lastDataFileFormatMetadata = Option.empty();
+ dataFileWriter.close();
+ if (writeConfig.isMetadataColumnStatsIndexEnabled()) {
+ try {
+ lastDataFileFormatMetadata =
Option.ofNullable(dataFileWriter.getFileFormatMetadata());
+ } catch (UnsupportedOperationException e) {
+ // File-format metadata is an optional writer capability.
Unsupported formats can still produce valid
+ // native log files; they simply do not contribute column stats
for this append.
+ lastDataFileFormatMetadata = Option.empty();
+ }
+ }
}
- } else {
- lastDataFileFormatMetadata = Option.empty();
+ } finally {
+ dataFileWriter = null;
Review Comment:
🤖 Now that the writers are nulled even when `flushAppend()` fails, the next
append will open a new writer. But `flushAppend` throws before it resets
`currentAppendVersion`, `dataLogFile` and `deleteLogFile`, so that new writer
would reuse the same version and path as the file whose close just failed. Is
reusing the writer after a failed flush meant to be supported (the new test
does this)? If so, should the failure path also reset `currentAppendVersion` so
we don't overwrite or collide with a partially written log file?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/io/cdc/HoodieNativeLogFormatWriter.java:
##########
@@ -277,27 +278,41 @@ private void ensureAppendVersion() throws IOException {
}
private void closeFileWriters() throws IOException {
- if (dataFileWriter != null) {
- dataFileWriter.close();
- if (writeConfig.isMetadataColumnStatsIndexEnabled()) {
- try {
- lastDataFileFormatMetadata =
Option.ofNullable(dataFileWriter.getFileFormatMetadata());
- } catch (UnsupportedOperationException e) {
- // File-format metadata is an optional writer capability.
Unsupported formats can still produce valid
- // native log files; they simply do not contribute column stats for
this append.
+ try {
Review Comment:
🤖 nit: closeFileWriters now nests try/try-finally/catch three levels deep,
which makes the control flow (especially where lastDataFileFormatMetadata gets
reset) harder to follow. Might be worth extracting the data-writer-close logic
into its own `closeDataFileWriter()` method (mirroring the new
`closeDeleteFileWriter()`) so this method just reads as "close data,
suppress-close delete on failure".
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
