rangareddy commented on code in PR #19415:
URL: https://github.com/apache/hudi/pull/19415#discussion_r3688065273
##########
hudi-common/src/test/java/org/apache/hudi/common/table/read/buffer/TestKeyBasedFileGroupRecordBuffer.java:
##########
@@ -74,6 +79,27 @@ class TestKeyBasedFileGroupRecordBuffer extends
BaseTestFileGroupRecordBuffer {
private final IndexedRecord testRecord6DeleteByCustomMarker =
createTestRecord("6", 3, 2L);
private final IndexedRecord testRecord7 = createTestRecord("7", 1, 5L);
+ /**
+ * Every buffer in this hierarchy must funnel records into the buffer
through the same
+ * merge-then-put path and expose the same partial-merge state, so these
methods are sealed with
+ * {@code final} and subclasses such as {@code
PositionBasedFileGroupRecordBuffer} and
+ * {@code SortedKeyBasedFileGroupRecordBuffer} cannot replace that behavior.
The methods that a
+ * subclass legitimately specializes (block processing, base-record
advancement, buffer type) are
+ * deliberately left open. This test fails if a {@code final} modifier is
dropped.
+ */
+ @Test
Review Comment:
Good catch, thank you — you're right on every point, and I've narrowed the
claim rather than invent the invariant. Addressed in 4c46e5ed3493.
Verified in the code before rewording anything:
- `FileGroupRecordBuffer.records` is `protected final`, so subclasses
genuinely do have direct access.
- `PositionBasedFileGroupRecordBuffer` writes to it directly in
`fallbackToKeyBasedBuffer` (re-keying position-keyed entries to record keys)
and in `processDeleteBlock` under `COMMIT_TIME_ORDERING`.
- One related overstatement you didn't mention, which I also fixed:
`enablePartialMerging` is `protected` and
`PositionBasedFileGroupRecordBuffer#processDataBlock` sets it, so the
`isPartialMergingEnabled` javadoc claiming the flag "is owned by the base
buffer" was wrong too.
I took the narrowing option rather than centralizing the mutations behind a
non-overridable helper, because the stronger funnel invariant isn't merely
unimplemented — implementing it would be incorrect. Both direct-write paths
must **not** merge: `fallbackToKeyBasedBuffer` re-keys records that have
already been merged, and the `COMMIT_TIME_ORDERING` delete path deliberately
overwrites, per the existing in-code comment about `put` vs `putIfAbsent` for
forward vs reverse log-file ordering. Routing either through
`processNextDataRecord` would change read behaviour.
So the scope is now exactly what `final` buys:
- test renamed `keyMergeBehaviorIsSealedAgainstSubclasses` →
`sealedMethodsCannotBeOverridden`
- its javadoc states the guarantee is only the two `final` modifiers, and
explicitly names the direct-write paths that bypass them and why they have to
- failure message trimmed to "must remain final so subclasses cannot
override it"
- the same correction applied to both production javadocs
Pushed as a separate commit rather than an amend so the delta since your
review is visible. 30 buffer tests plus checkstyle are green, and I
re-confirmed the renamed test still fails when either `final` is dropped.
If you would still like the stronger invariant, the tractable version is a
`final` helper covering raw map mutation only (not merging) — happy to do that
as a follow-up if you think it's worth the churn.
--
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]
