voonhous commented on code in PR #19464:
URL: https://github.com/apache/hudi/pull/19464#discussion_r3703451715
##########
hudi-common/src/main/java/org/apache/hudi/common/util/ClusteringUtils.java:
##########
@@ -121,14 +121,18 @@ public static Option<HoodieInstant>
getRequestedClusteringInstant(String timesta
/**
* Transitions the provided clustering instant fron inflight to complete
based on the clustering
* action type. After HUDI-7905, the new clustering commits are written with
clustering action.
+ *
+ * @return the completed instant, whose action is the one recorded on the
timeline. This differs
+ * from the inflight action: a {@code clustering} inflight instant
completes as
+ * {@code replacecommit}.
*/
- public static <T> void
transitionClusteringOrReplaceInflightToComplete(boolean shouldLock,
HoodieInstant clusteringInstant,
-
HoodieReplaceCommitMetadata metadata, HoodieActiveTimeline activeTimeline,
-
TableFormatCompletionAction tableFormatCompletionAction) {
+ public static <T> HoodieInstant
transitionClusteringOrReplaceInflightToComplete(boolean shouldLock,
HoodieInstant clusteringInstant,
+
HoodieReplaceCommitMetadata metadata, HoodieActiveTimeline activeTimeline,
+
TableFormatCompletionAction tableFormatCompletionAction) {
if
(clusteringInstant.getAction().equals(HoodieTimeline.CLUSTERING_ACTION)) {
- activeTimeline.transitionClusterInflightToComplete(shouldLock,
clusteringInstant, metadata, tableFormatCompletionAction);
+ return activeTimeline.transitionClusterInflightToComplete(shouldLock,
clusteringInstant, metadata, tableFormatCompletionAction);
Review Comment:
The contract this javadoc now states -- a `clustering` inflight completes as
`replacecommit` -- is not pinned by any test in the module that owns it. The
Flink test mocks this method away entirely
(`TestHoodieFlinkTableServiceClient.java:256-258`) and the java-client
functional test only observes it three layers down through the callback. Both
would still pass if the completed action here ever changed.
That matters more than usual because this is the third time this bug class
has landed (`bf2b712c1c48` / HUDI-6384, `1338e2998d58` / HUDI-7161,
`bf3cc4393981` / #18988) and the first time it has had any guard.
`TestClusteringUtils.java:125-126` already drives a real timeline through
this transition and holds the completed instant, but asserts only the state:
```java
HoodieInstant complete =
metaClient.getActiveTimeline().transitionClusterInflightToComplete(false,
inflight, new HoodieReplaceCommitMetadata());
assertEquals(HoodieInstant.State.COMPLETED, complete.getState());
```
Please add `assertEquals(HoodieTimeline.REPLACE_COMMIT_ACTION,
complete.getAction())` there, plus a case that calls
`ClusteringUtils.transitionClusteringOrReplaceInflightToComplete` directly for
both inflight kinds (`clustering` and `replacecommit`) and asserts
`replacecommit` for each. The `replacecommit` case also covers the `else`
branch on line 135 -- the one a table-version <= 7 writer takes -- which
neither new test reaches today.
##########
hudi-client/hudi-flink-client/src/test/java/org/apache/hudi/client/TestHoodieFlinkTableServiceClient.java:
##########
@@ -218,15 +225,24 @@ void testCompleteCompactionCommitsAndCleansMarkers() {
@Test
void testCompleteClusteringCommitsAndCleansMarkers() {
+ ClusteringCallback.MESSAGES.clear();
HoodieWriteConfig writeConfig = HoodieWriteConfig.newBuilder()
.withPath(metaClient.getBasePath())
.withMetadataConfig(HoodieMetadataConfig.newBuilder().enable(false).build())
+ .withCallbackConfig(HoodieWriteCommitCallbackConfig.newBuilder()
+ .writeCommitCallbackOn("true")
+ .withCallbackClass(ClusteringCallback.class.getName())
+ .build())
.build();
HoodieFlinkTable table = mock(HoodieFlinkTable.class);
HoodieActiveTimeline activeTimeline = mock(HoodieActiveTimeline.class);
when(table.getActiveTimeline()).thenReturn(activeTimeline);
when(table.getInstantGenerator()).thenReturn(metaClient.getInstantGenerator());
HoodieInstant clusteringInstant = mock(HoodieInstant.class);
Review Comment:
This test does catch a revert, but by accident rather than by design.
`clusteringInstant` is never stubbed, so against the pre-fix code
`clusteringInstant.getAction()` returns null, `Option.of(null)` throws
(`Option.java:65`), `fireCommitCallbackIfNecessary` swallows it
(`BaseHoodieClient.java:505-506`), and `MESSAGES` ends up empty. So it is the
size assertion on line 266 that fails, with `expected: <1> but was: <0>`, which
does not name the bug. The action assertion on line 267 is tautological -- it
reads back exactly what line 245 stubbed.
One line makes the mock match reality (a v2 inflight really is `clustering`)
and turns a revert into the failure that names the defect:
```suggestion
HoodieInstant clusteringInstant = mock(HoodieInstant.class);
when(clusteringInstant.getAction()).thenReturn(HoodieTimeline.CLUSTERING_ACTION);
```
No strictness concern: this class has no
`@ExtendWith(MockitoExtension.class)`, so the stub going unused on the fixed
path will not trip `UnnecessaryStubbingException`.
##########
hudi-common/src/main/java/org/apache/hudi/common/util/ClusteringUtils.java:
##########
@@ -121,14 +121,18 @@ public static Option<HoodieInstant>
getRequestedClusteringInstant(String timesta
/**
* Transitions the provided clustering instant fron inflight to complete
based on the clustering
* action type. After HUDI-7905, the new clustering commits are written with
clustering action.
+ *
+ * @return the completed instant, whose action is the one recorded on the
timeline. This differs
+ * from the inflight action: a {@code clustering} inflight instant
completes as
+ * {@code replacecommit}.
*/
- public static <T> void
transitionClusteringOrReplaceInflightToComplete(boolean shouldLock,
HoodieInstant clusteringInstant,
-
HoodieReplaceCommitMetadata metadata, HoodieActiveTimeline activeTimeline,
-
TableFormatCompletionAction tableFormatCompletionAction) {
+ public static <T> HoodieInstant
transitionClusteringOrReplaceInflightToComplete(boolean shouldLock,
HoodieInstant clusteringInstant,
Review Comment:
nit, feel free to ignore: the `<T>` type parameter is unused, and now that
the method returns `HoodieInstant` there is no reading under which it could
bind. All three call sites are plain calls with no type witness, so dropping it
is safe:
```suggestion
public static HoodieInstant
transitionClusteringOrReplaceInflightToComplete(boolean shouldLock,
HoodieInstant clusteringInstant,
HoodieReplaceCommitMetadata metadata, HoodieActiveTimeline activeTimeline,
TableFormatCompletionAction tableFormatCompletionAction) {
```
The sibling `transitionClusteringOrReplaceRequestedToInflight` on line 143
does legitimately need its `<T>` -- leave that one alone.
##########
hudi-client/hudi-flink-client/src/test/java/org/apache/hudi/client/TestHoodieFlinkTableServiceClient.java:
##########
@@ -237,13 +253,33 @@ void testCompleteClusteringCommitsAndCleansMarkers() {
clusteringUtils.when(() -> ClusteringUtils.getInflightClusteringInstant(
"20260723120000001", activeTimeline,
metaClient.getInstantGenerator()))
.thenReturn(Option.of(clusteringInstant));
+ clusteringUtils.when(() ->
ClusteringUtils.transitionClusteringOrReplaceInflightToComplete(
+ anyBoolean(), any(), any(), any(), any()))
+ .thenReturn(completedInstant);
markersFactory.when(() -> WriteMarkersFactory.get(any(), any(),
any())).thenReturn(writeMarkers);
client.callCompleteClustering(metadata, table, "20260723120000001");
} finally {
client.close();
}
verify(writeMarkers).quietDeleteMarkerDir(any(), any(Integer.class));
+ assertEquals(1, ClusteringCallback.MESSAGES.size(), "callback must fire
once for the clustering commit");
+ assertEquals(HoodieTimeline.REPLACE_COMMIT_ACTION,
+ ClusteringCallback.MESSAGES.get(0).getCommitActionType().orElse(null));
+ }
+
+ public static class ClusteringCallback implements HoodieWriteCommitCallback {
Review Comment:
nit, feel free to ignore: `ClusteringCallback` is byte-for-byte identical to
`RecordingCommitCallback` in
`TestHoodieJavaClientOnMergeOnReadStorage.java:289` -- same static
`CopyOnWriteArrayList`, same reflective constructor, same `// config arg
required for reflective instantiation` comment.
Both modules already consume the client-common test-jar
(`hudi-client/hudi-flink-client/pom.xml:151-155` and
`hudi-client/hudi-java-client/pom.xml:96-100`), so this can move into
hudi-client-common test sources with no pom changes and both tests can share
one copy. A third copy is likely the next time someone tests a callback.
##########
hudi-client/hudi-java-client/src/test/java/org/apache/hudi/client/functional/TestHoodieJavaClientOnMergeOnReadStorage.java:
##########
@@ -270,15 +270,14 @@ public void testWriteCommitCallbackFiresOnClustering()
throws Exception {
client.cluster(clusteringTime.get(), true);
assertTrue(metaClient.reloadActiveTimeline().filterCompletedInstants().containsInstant(clusteringTime.get()));
- // The callback must fire once for the clustering completion, reporting
the action actually on
- // the timeline (replacecommit for table version < 8, clustering for 8+).
+ // The callback must fire exactly once for the clustering completion,
reporting the completed
+ // timeline action (replacecommit).
List<HoodieWriteCommitCallbackMessage> clusteringMessages =
RecordingCommitCallback.MESSAGES.stream()
.filter(m -> m.getCommitTime().equals(clusteringTime.get()))
.collect(Collectors.toList());
assertEquals(1, clusteringMessages.size(), "callback must fire once for
the clustering commit");
- String action =
clusteringMessages.get(0).getCommitActionType().orElse(null);
- assertTrue(HoodieTimeline.REPLACE_COMMIT_ACTION.equals(action) ||
HoodieTimeline.CLUSTERING_ACTION.equals(action),
- "clustering callback must report the timeline action, got: " + action);
+ assertEquals(HoodieTimeline.REPLACE_COMMIT_ACTION,
clusteringMessages.get(0).getCommitActionType().orElse(null));
+ assertNotNull(clusteringMessages.get(0).getPrevFilePaths(), "prevFilePaths
must never be null");
Review Comment:
nit: this assertion cannot fail, and it is not related to the action-type
fix this PR is making. `getPrevFilePaths()` falls back to
`Collections.emptyMap()` (`HoodieWriteCommitCallbackMessage.java:158`) and
`resolvePrevFilePaths` never returns null, so non-nullness is structural. It is
also a verbatim duplicate of line 236 in this same file, and the invariant
already has dedicated coverage in client-common at
`TestHoodieWriteCommitCallbackMessage.java:94` and `:119`.
If you want the line to earn its place, assert the contents instead. For a
clustering commit the map is guaranteed *empty*: clustering writes new file
groups, so every stat carries `NULL_COMMIT` and
`HoodieWriteCommitCallbackUtil.java:74` skips it.
```suggestion
// Clustering writes new file groups, so every stat carries NULL_COMMIT
and no prev path resolves.
assertTrue(clusteringMessages.get(0).getPrevFilePaths().isEmpty());
```
Otherwise just drop the line.
--
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]
