fix(client): create a new log file version instead of reopening the latest on pre-v8 writes - #19755
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19755 +/- ##
============================================
- Coverage 77.96% 76.15% -1.82%
+ Complexity 33458 32689 -769
============================================
Files 2539 2539
Lines 140939 140942 +3
Branches 17012 17012
============================================
- Hits 109890 107336 -2554
- Misses 23388 25851 +2463
- Partials 7661 7755 +94
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR changes the pre-v8 createLogWriter path to target a new log file version (latest + 1) rather than reopening the latest log file, avoiding a dependency on append support. Tracing the write path, the end-state file matches what the old exists()→rollover path produced, so the change looks behavior-preserving. One minor consistency question on the initial file size is noted inline. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. A couple of minor naming nits in the new test file; the production change looks clean.
| .map(logFile -> logFile.getLogVersion() + 1) | ||
| .orElse(HoodieLogFile.LOGFILE_BASE_VERSION); | ||
| return HoodieLogFormatWriter.builder() | ||
| .withParentPath(FSUtils.constructAbsolutePath(hoodieTable.getMetaClient().getBasePath(), partitionPath)) |
There was a problem hiding this comment.
🤖 Now that this targets a fresh version (latest + 1) rather than reopening the latest file, should withFileSize be 0L to match the v8+ path above? Passing the previous log file's size seeds HoodieLogFile.getFileSize() on a file that actually starts empty. It looks harmless today (rollover uses outputStream.getPos(), and write stats use result.size()), but the stale initial size is a bit inconsistent with the intent.
| cleanMetaClient(); | ||
| } | ||
|
|
||
| private void mockMethodsNeededByConstructor() { |
There was a problem hiding this comment.
🤖 nit: mockMethodsNeededByConstructor reads as an explanation of why rather than a name — could you rename it to something like stubTableMocks() or initMockTable()?
| when(mockHoodieTable.getMetaClient()).thenReturn(metaClient); | ||
| } | ||
|
|
||
| private static Stream<Arguments> versionsSixAndAbove() { |
There was a problem hiding this comment.
🤖 nit: versionsSixAndAbove implies it covers all table versions ≥ 6, but only three specific ones are listed — something like preAndPostV8Versions() or tableVersionsUnderTest() would be more accurate and make it clear why SEVEN (if it exists) is absent.
…atest on pre-v8 writes
For a pre-table-version-8 table, HoodieWriteHandle.createLogWriter opened the log
writer at the latest existing log file's version (reopening/append-targeting it).
Target a new version (latest + 1) instead, so the write does not depend on append
support for the latest log file. The table-version-8+ path derives log file names
differently and is unaffected.
Add TestHoodieAppendHandle.testCreateLogFileWriterLogVersion, parameterized over
{SIX, EIGHT, NINE}: pre-v8 increments the version when a log file is present, v8+ is
unchanged. Mirrors the equivalent change on the Onehouse internal 1.x line.
The test does not compile as cherry-picked from the internal 1.x line: on apache/hudi master HoodieAppendHandle is abstract, so instantiate the concrete HoodieInlineLogAppendHandle; and HoodieStorageUtils lives in org.apache.hudi.common.util (not org.apache.hudi.storage).
b494716 to
5f9e6ca
Compare
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR changes the pre-v8 log-writer path so it targets a fresh log file version (latest + 1) instead of reopening the latest, avoiding reliance on append/exists semantics; the v8+ path is untouched. I traced the change through HoodieLogFormatWriter and confirmed the version selection, write-token reuse, and rollover behavior are consistent with the existing rollover path. The one open item (the withFileSize still carrying the old file's size) was already raised in a prior round's inline comment and is worth confirming. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. One small naming nit in the test helper, otherwise the code is clean and readable.
cc @yihua
| cleanMetaClient(); | ||
| } | ||
|
|
||
| private void mockMethodsNeededByConstructor() { |
There was a problem hiding this comment.
🤖 nit: could you rename this to something like stubTableDefaults() or mockTableConfig()? mockMethodsNeededByConstructor describes why the helper exists rather than what it stubs, which makes it a little harder to read at the call site in setUp.
…atest on pre-v8 writes (apache#19755) (cherry picked from commit b5263e1)
…in TestHoodieAppendHandle The apache#19755 backport (4b71100) brought the test over with master's org.apache.hudi.common.util.HoodieStorageUtils import. The hudi-common package reorg (apache#19195) is not on this branch, so the class only exists as org.apache.hudi.storage.HoodieStorageUtils here and hudi-client-common test sources did not compile. Import-only change.
…andle on this branch The apache#19755 backport (4b71100) instantiates HoodieInlineLogAppendHandle, which master introduced with the native-format log files (apache#19067). That split is not on this branch, where HoodieAppendHandle is still the concrete class with the same six-argument constructor, so the test constructs it directly. Test-only change.
5 participants
Describe the issue this Pull Request addresses
closes #19754
Summary and Changelog
For a pre-table-version-8 table,
HoodieWriteHandle#createLogWriteropened the log writer at the latest existing log file's version (reopening/append-targeting it). This changes it to target a new version (latest + 1) so the write does not depend on append support for the latest log file. The table-version-8+ path derives log file names differently and is unaffected.Adds
TestHoodieAppendHandle.testCreateLogFileWriterLogVersion, parameterized over table versions {SIX, EIGHT, NINE}, asserting the pre-v8 writer increments the version when a log file is present and the v8+ path is unchanged.Mirrors the equivalent change on the Onehouse internal 1.x line.
Impact
Pre-v8 MOR log writes create a new log file version instead of reopening the latest one. The v8+ path is unchanged.
Risk Level
low. Confined to pre-v8 log-writer version selection and covered by a unit test.
Documentation Update
none
Contributor's checklist