voonhous commented on code in PR #19865:
URL: https://github.com/apache/hudi/pull/19865#discussion_r3964757837
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/common/util/TestClusteringUtils.java:
##########
@@ -182,7 +180,7 @@ public void testClusteringPlanInflight() throws Exception {
fileIds1.add(UUID.randomUUID().toString());
String clusterTime1 = "1";
HoodieInstant requestedInstant =
createRequestedClusterInstant(partitionPath1, clusterTime1, fileIds1);
- HoodieInstant inflightInstant =
metaClient.getActiveTimeline().transitionReplaceRequestedToInflight(requestedInstant,
Option.empty());
+ HoodieInstant inflightInstant =
metaClient.getActiveTimeline().transitionClusterRequestedToInflight(requestedInstant,
Option.empty());
assertTrue(ClusteringUtils.isClusteringInstant(metaClient.getActiveTimeline(),
requestedInstant, INSTANT_GENERATOR));
Review Comment:
**major:** both instants are now `clustering`, so this assert (and line 186)
short-circuits on the action string at `ClusteringUtils.java:168` and never
reads a plan. The class thereby loses its only pin on the
`REPLACE_COMMIT_ACTION && getClusteringPlan(...).isPresent()` arm: delete that
disjunct and all 11 tests stay green. The arm is live for table-version-6
tables, which `ClusteringPlanActionExecutor.java:93-94` still schedules as
`replacecommit`, and it gates conflict resolution and rollback.
Could we keep this case and add the replacecommit sibling the pre-#11553
test had: `saveToPendingReplaceCommit` with a `CLUSTER` plan under a
`REPLACE_COMMIT_ACTION` requested instant,
`transitionReplaceRequestedToInflight`, then the same two asserts? On the
default metaClient that passes today and fails with the disjunct removed.
<details>
<summary>callers and history</summary>
- gated by the arm: `PreferWriterConflictResolutionStrategy:70/142`,
`SimpleConcurrentFileWritesConflictResolutionStrategy:128`,
`BaseRollbackActionExecutor:199/225`, `BaseHoodieTableServiceClient:985/1038`,
every filter in `BaseTimelineV1`
- `ActiveTimelineV1:472` aliases `transitionClusterRequestedToInflight` to
the replace transition, so V1 timelines never carry `clustering`
- HUDI-8610 (#12375, `56159e2855e7`) changed this arm with 0 test files;
#18288 (`f64c93ee899c`) was motivated by replacecommits but its 4 tests all use
`CLUSTERING_ACTION`
- #11553 (`6456d7796006`) deleted the `createRequestedReplaceInstant` helper
this test used, so nothing in the class can build that shape any more
</details>
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/common/util/TestClusteringUtils.java:
##########
@@ -182,7 +180,7 @@ public void testClusteringPlanInflight() throws Exception {
fileIds1.add(UUID.randomUUID().toString());
String clusterTime1 = "1";
HoodieInstant requestedInstant =
createRequestedClusterInstant(partitionPath1, clusterTime1, fileIds1);
- HoodieInstant inflightInstant =
metaClient.getActiveTimeline().transitionReplaceRequestedToInflight(requestedInstant,
Option.empty());
+ HoodieInstant inflightInstant =
metaClient.getActiveTimeline().transitionClusterRequestedToInflight(requestedInstant,
Option.empty());
assertTrue(ClusteringUtils.isClusteringInstant(metaClient.getActiveTimeline(),
requestedInstant, INSTANT_GENERATOR));
HoodieClusteringPlan requestedClusteringPlan =
ClusteringUtils.getClusteringPlan(metaClient,
requestedInstant).get().getRight();
assertTrue(ClusteringUtils.isClusteringInstant(metaClient.getActiveTimeline(),
inflightInstant, INSTANT_GENERATOR));
Review Comment:
**minor:** not blocking. Every test in the class now drives `clustering`
instants only in requested or inflight state, so the completed-instant `else`
at `ClusteringUtils.java:190-192` (the `InstantGeneratorV2 ? CLUSTERING :
REPLACE_COMMIT` fork HUDI-8610 added without a test) is never entered;
`testGetOldestInstantToRetainForClustering` completes instants but never calls
`getClusteringPlan`.
Would it be worth completing this instant with
`transitionClusterInflightToComplete` and asserting
`getClusteringPlan(metaClient, completed)` still returns
`requestedClusteringPlan`? Three lines, and the only unit coverage that fork
would have.
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/common/util/TestClusteringUtils.java:
##########
@@ -182,7 +180,7 @@ public void testClusteringPlanInflight() throws Exception {
fileIds1.add(UUID.randomUUID().toString());
String clusterTime1 = "1";
HoodieInstant requestedInstant =
createRequestedClusterInstant(partitionPath1, clusterTime1, fileIds1);
- HoodieInstant inflightInstant =
metaClient.getActiveTimeline().transitionReplaceRequestedToInflight(requestedInstant,
Option.empty());
+ HoodieInstant inflightInstant =
metaClient.getActiveTimeline().transitionClusterRequestedToInflight(requestedInstant,
Option.empty());
Review Comment:
**nit:** feel free to ignore. The new comment says the inflight file carries
no plan, but nothing checks it; the redirect at `ClusteringUtils.java:187` is
unconditional, so the test passes identically if the inflight file did hold
one. Could we pin the premise?
```suggestion
HoodieInstant inflightInstant =
metaClient.getActiveTimeline().transitionClusterRequestedToInflight(requestedInstant,
Option.empty());
assertTrue(metaClient.getActiveTimeline().isEmpty(inflightInstant));
```
##########
hudi-hadoop-common/src/test/java/org/apache/hudi/common/util/TestClusteringUtils.java:
##########
@@ -171,9 +170,8 @@ false, replaceInflight, new HoodieReplaceCommitMetadata(),
metaClient.getActiveT
assertEquals(HoodieTimeline.REPLACE_COMMIT_ACTION,
instant.getAction()));
}
- // replacecommit.inflight doesn't have clustering plan.
- // Verify that getClusteringPlan fetches content from corresponding
requested file.
- @Disabled("Will fail due to avro issue AVRO-3789. This is fixed in avro
1.11.3")
Review Comment:
**nit:** feel free to ignore. The body says the avro reason was stale, which
reads as wrong from day one. It was right when this annotation landed in #9717
(`874b5dec5e9d`, 2023-11); the transition was still legal then. #11553
(HUDI-7905, `6456d7796006`, 2024-07) later migrated every other
`transitionReplaceRequestedToInflight` in this file and deleted the
`createRequestedReplaceInstant` helper, skipping this test because it was
disabled. Would it be worth one line in the body naming #11553 as where the
mismatch entered, so a committer sees this as the last leftover of that
migration?
--
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]
