You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "codope (via GitHub)" <gi...@apache.org> on 2026/08/03 05:30:45 UTC
[PR] fix(client): report the completed timeline action in the clustering w… [hudi]
codope opened a new pull request, #19464:
URL: https://github.com/apache/hudi/pull/19464
### Describe the issue this Pull Request addresses
Follow-up to #18988. `completeClustering` fires the write commit callback with
`clusteringInstant.getAction()`, but that is the **inflight** instant, captured before the
transition and immutable. On timeline v2 its action is `clustering`, while
`transitionClusterInflightToComplete` records the completed instant as `replacecommit`. So on table
version 8+ the callback reports an action that does not exist on the completed timeline. On the 0.x
layout the two coincide, which is why it went unnoticed.
### Summary and Changelog
Report the action the commit lands under, not the one it was scheduled under.
- `ClusteringUtils.transitionClusteringOrReplaceInflightToComplete` now returns the completed
`HoodieInstant`. Both delegates already returned it; the util discarded it.
- `BaseHoodieTableServiceClient#completeClustering` and
`HoodieFlinkTableServiceClient#completeClustering` pass that instant's action to the callback.
This keeps the completed action defined only in the timeline implementation, so it cannot drift.
The sibling call sites are already correct: compaction fires `COMMIT_ACTION`, log compaction
`DELTA_COMMIT_ACTION`.
- `TestHoodieJavaClientOnMergeOnReadStorage#testWriteCommitCallbackFiresOnClustering` asserted
`REPLACE_COMMIT_ACTION || CLUSTERING_ACTION`, which accepted the wrong value and could not catch
this; tightened to `assertEquals`, mirroring the compaction test.
- `TestHoodieFlinkTableServiceClient#testCompleteClusteringCommitsAndCleansMarkers` statically mocks
`ClusteringUtils`, so the transition is stubbed to return a completed instant, and the test now
also asserts the Flink path reports `replacecommit` — that fire previously had no coverage.
### Impact
Consumers of `HoodieWriteCommitCallback` on table version 8+ see `replacecommit` instead of
`clustering` for clustering commits — the value the timeline records, and the value already reported
on older table versions.
`transitionClusteringOrReplaceInflightToComplete` changes from `void` to returning `HoodieInstant`.
It is not annotated public API and has two callers, both in this PR, so this is source-compatible
in-tree; a pre-compiled out-of-tree caller would need recompiling.
### Risk Level
low — no change to when or whether the callback fires, or to what is written to the timeline.
### Documentation Update
none
### Contributor's checklist
- [x] Read through [contributor's guide](https://hudi.apache.org/contribute/how-to-contribute)
- [x] Enough context is provided in the sections above
- [x] Adequate tests were added if applicable
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report completed timeline action in clustering callback [hudi]
Posted by "codope (via GitHub)" <gi...@apache.org>.
codope commented on code in PR #19464:
URL: https://github.com/apache/hudi/pull/19464#discussion_r3709584509
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java:
##########
@@ -644,7 +645,7 @@ private void completeClustering(HoodieReplaceCommitMetadata replaceCommitMetadat
heartbeatClient.stop(clusteringCommitTime);
}
log.info("Clustering successfully on commit {} for table {}", clusteringCommitTime, table.getConfig().getBasePath());
- fireCommitCallbackIfNecessary(clusteringCommitTime, clusteringInstant.getAction(),
+ fireCommitCallbackIfNecessary(clusteringCommitTime, completedClusteringInstant.getAction(),
Review Comment:
Agreed it's a marker, but the marker every other operation uses is the *completed* action, not the operation kind:
- compaction fires [COMMIT_ACTION](https://github.com/apache/hudi/blob/e4718be307900b5348f26bc113885f224c33896c/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java#L427) even though its inflight is `compaction`
- log compaction fires [DELTA_COMMIT_ACTION](https://github.com/apache/hudi/blob/e4718be307900b5348f26bc113885f224c33896c/hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java#L501) even though its inflight is `logcompaction`
- insert_overwrite / insert_overwrite_table / delete_partition reach the callback through
`postCommit` with [REPLACE_COMMIT_ACTION](https://github.com/apache/hudi/blob/e4718be307900b5348f26bc113885f224c33896c/hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/table/action/commit/SparkInsertOverwriteCommitActionExecutor.java#L82)
- clustering on table version <= 6 already reports `replacecommit`, and did so before HUDI-7905 split
the inflight action out
Hardcoding `CLUSTERING_ACTION` would make clustering the only operation whose callback reports an
action with no completed instant behind it, and would silently change what a version-8/9 table reports
relative to an older one.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report completed timeline action in clustering callback [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19464:
URL: https://github.com/apache/hudi/pull/19464#issuecomment-5176463216
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15959",
"triggerID" : "4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28",
"triggerType" : "PUSH"
}, {
"hash" : "da413df0cc77c92c77978c121bb4f891714edb92",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16026",
"triggerID" : "da413df0cc77c92c77978c121bb4f891714edb92",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* da413df0cc77c92c77978c121bb4f891714edb92 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16026)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report the completed timeline action in the clustering w… [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19464:
URL: https://github.com/apache/hudi/pull/19464#issuecomment-5162707927
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report completed timeline action in clustering callback [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous merged PR #19464:
URL: https://github.com/apache/hudi/pull/19464
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report the completed timeline action in the clustering w… [hudi]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19464:
URL: https://github.com/apache/hudi/pull/19464#issuecomment-5162761898
## [Codecov](https://app.codecov.io/gh/apache/hudi/pull/19464?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `75.00000%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 76.81%. Comparing base ([`64afac0`](https://app.codecov.io/gh/apache/hudi/commit/64afac03908bba1f32df05982aee388d9011b714?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`4b28420`](https://app.codecov.io/gh/apache/hudi/commit/4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/hudi/pull/19464?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...a/org/apache/hudi/common/util/ClusteringUtils.java](https://app.codecov.io/gh/apache/hudi/pull/19464?src=pr&el=tree&filepath=hudi-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fcommon%2Futil%2FClusteringUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3V0aWwvQ2x1c3RlcmluZ1V0aWxzLmphdmE=) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/hudi/pull/19464?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19464 +/- ##
============================================
- Coverage 76.81% 76.81% -0.01%
Complexity 33770 33770
============================================
Files 2576 2576
Lines 143457 143455 -2
Branches 17589 17589
============================================
- Hits 110201 110198 -3
- Misses 25010 25011 +1
Partials 8246 8246
```
| [Components](https://app.codecov.io/gh/apache/hudi/pull/19464/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hudi-common](https://app.codecov.io/gh/apache/hudi/pull/19464/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `82.26% <50.00%> (-0.01%)` | :arrow_down: |
| [hudi-client](https://app.codecov.io/gh/apache/hudi/pull/19464/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.85% <100.00%> (ø)` | |
| [hudi-flink](https://app.codecov.io/gh/apache/hudi/pull/19464/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `83.96% <ø> (-0.01%)` | :arrow_down: |
| [hudi-spark-datasource](https://app.codecov.io/gh/apache/hudi/pull/19464/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `74.22% <ø> (ø)` | |
| [hudi-utilities](https://app.codecov.io/gh/apache/hudi/pull/19464/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.66% <ø> (ø)` | |
| [hudi-cli](https://app.codecov.io/gh/apache/hudi/pull/19464/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `15.32% <ø> (ø)` | |
| [hudi-hadoop](https://app.codecov.io/gh/apache/hudi/pull/19464/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `63.49% <ø> (ø)` | |
| [hudi-sync](https://app.codecov.io/gh/apache/hudi/pull/19464/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.87% <ø> (ø)` | |
| [hudi-io](https://app.codecov.io/gh/apache/hudi/pull/19464/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `79.60% <ø> (ø)` | |
| [hudi-timeline-service](https://app.codecov.io/gh/apache/hudi/pull/19464/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `84.23% <ø> (ø)` | |
| [hudi-cloud](https://app.codecov.io/gh/apache/hudi/pull/19464/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `64.00% <ø> (ø)` | |
| [hudi-kafka-connect](https://app.codecov.io/gh/apache/hudi/pull/19464/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.20% <ø> (ø)` | |
| [Flag](https://app.codecov.io/gh/apache/hudi/pull/19464/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hadoop-mr-java-client](https://app.codecov.io/gh/apache/hudi/pull/19464/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `43.36% <75.00%> (-0.06%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files with missing lines](https://app.codecov.io/gh/apache/hudi/pull/19464?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ache/hudi/client/BaseHoodieTableServiceClient.java](https://app.codecov.io/gh/apache/hudi/pull/19464?src=pr&el=tree&filepath=hudi-client%2Fhudi-client-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fclient%2FBaseHoodieTableServiceClient.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jbGllbnQvaHVkaS1jbGllbnQtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWRpL2NsaWVudC9CYXNlSG9vZGllVGFibGVTZXJ2aWNlQ2xpZW50LmphdmE=) | `75.33% <100.00%> (ø)` | |
| [...che/hudi/client/HoodieFlinkTableServiceClient.java](https://app.codecov.io/gh/apache/hudi/pull/19464?src=pr&el=tree&filepath=hudi-client%2Fhudi-flink-client%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fclient%2FHoodieFlinkTableServiceClient.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jbGllbnQvaHVkaS1mbGluay1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY2xpZW50L0hvb2RpZUZsaW5rVGFibGVTZXJ2aWNlQ2xpZW50LmphdmE=) | `72.61% <ø> (-0.64%)` | :arrow_down: |
| [...a/org/apache/hudi/common/util/ClusteringUtils.java](https://app.codecov.io/gh/apache/hudi/pull/19464?src=pr&el=tree&filepath=hudi-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fcommon%2Futil%2FClusteringUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3V0aWwvQ2x1c3RlcmluZ1V0aWxzLmphdmE=) | `83.23% <50.00%> (-0.58%)` | :arrow_down: |
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report the completed timeline action in the clustering w… [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19464:
URL: https://github.com/apache/hudi/pull/19464#issuecomment-5162736370
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15959",
"triggerID" : "4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15959)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report completed timeline action in clustering callback [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19464:
URL: https://github.com/apache/hudi/pull/19464#issuecomment-5174827274
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15959",
"triggerID" : "4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28",
"triggerType" : "PUSH"
}, {
"hash" : "da413df0cc77c92c77978c121bb4f891714edb92",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16026",
"triggerID" : "da413df0cc77c92c77978c121bb4f891714edb92",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15959)
* da413df0cc77c92c77978c121bb4f891714edb92 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16026)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report completed timeline action in clustering callback [hudi]
Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #19464:
URL: https://github.com/apache/hudi/pull/19464#discussion_r3718858720
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java:
##########
@@ -644,7 +645,7 @@ private void completeClustering(HoodieReplaceCommitMetadata replaceCommitMetadat
heartbeatClient.stop(clusteringCommitTime);
}
log.info("Clustering successfully on commit {} for table {}", clusteringCommitTime, table.getConfig().getBasePath());
- fireCommitCallbackIfNecessary(clusteringCommitTime, clusteringInstant.getAction(),
+ fireCommitCallbackIfNecessary(clusteringCommitTime, completedClusteringInstant.getAction(),
Review Comment:
yeah, I prefer the constant to keep it simple, the action type has ambiguities. the action switch between different state is really confusing.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report completed timeline action in clustering callback [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19464:
URL: https://github.com/apache/hudi/pull/19464#issuecomment-5165867650
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15959",
"triggerID" : "4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15959)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report completed timeline action in clustering callback [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report the completed timeline action in the clustering w… [hudi]
Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #19464:
URL: https://github.com/apache/hudi/pull/19464#discussion_r3701824284
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java:
##########
@@ -644,7 +645,7 @@ private void completeClustering(HoodieReplaceCommitMetadata replaceCommitMetadat
heartbeatClient.stop(clusteringCommitTime);
}
log.info("Clustering successfully on commit {} for table {}", clusteringCommitTime, table.getConfig().getBasePath());
- fireCommitCallbackIfNecessary(clusteringCommitTime, clusteringInstant.getAction(),
+ fireCommitCallbackIfNecessary(clusteringCommitTime, completedClusteringInstant.getAction(),
Review Comment:
can we just pass around `HoodieActiveTimeline.CLUSTERING_ACTION` just like what we do to compaction or the clustering metrics reporting.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report the completed timeline action in the clustering w… [hudi]
Posted by "codope (via GitHub)" <gi...@apache.org>.
codope commented on code in PR #19464:
URL: https://github.com/apache/hudi/pull/19464#discussion_r3702901647
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java:
##########
@@ -644,7 +645,7 @@ private void completeClustering(HoodieReplaceCommitMetadata replaceCommitMetadat
heartbeatClient.stop(clusteringCommitTime);
}
log.info("Clustering successfully on commit {} for table {}", clusteringCommitTime, table.getConfig().getBasePath());
- fireCommitCallbackIfNecessary(clusteringCommitTime, clusteringInstant.getAction(),
+ fireCommitCallbackIfNecessary(clusteringCommitTime, completedClusteringInstant.getAction(),
Review Comment:
I don't think we can. The `CLUSTERING_ACTION` is used to build the metric key name (`getMetricsName(actionType, ...)` etc.). So, it's a label for the metric namespace, not necessarily about what's on the timeline. The callback's `commitActionType` is different: its whole contract (and the reason this PR and #18988 exist) is to report the action the commit actually completed under, so downstream consumers can correctly interpret the commit. On table version 8+ / layout v2, the completed instant is replacecommit even though the inflight one is clustering (that's the bug). If we hardcode `CLUSTERING_ACTION`, we'd reintroduce the issue and report clustering for a commit whose timeline entry is actually replacecommit.
I did think of hardcode `REPLACE_COMMIT_ACTION` at the two call sites, but that duplicates the completed-action mapping across ActiveTimelineV1/V2 and both call sites, and would go stale silently if that mapping ever changes. Returning the completed instant from `transitionClusteringOrReplaceInflightToComplete` keeps that mapping defined in exactly one place which is reusable, so I'd prefer to keep it.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report completed timeline action in clustering callback [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19464:
URL: https://github.com/apache/hudi/pull/19464#issuecomment-5192468503
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15959",
"triggerID" : "4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28",
"triggerType" : "PUSH"
}, {
"hash" : "da413df0cc77c92c77978c121bb4f891714edb92",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16026",
"triggerID" : "da413df0cc77c92c77978c121bb4f891714edb92",
"triggerType" : "PUSH"
}, {
"hash" : "efab1ab82b36d8cdcb0f56e566c86f13d7661b81",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16092",
"triggerID" : "efab1ab82b36d8cdcb0f56e566c86f13d7661b81",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* da413df0cc77c92c77978c121bb4f891714edb92 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16026)
* efab1ab82b36d8cdcb0f56e566c86f13d7661b81 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16092)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report completed timeline action in clustering callback [hudi]
Posted by "codope (via GitHub)" <gi...@apache.org>.
codope commented on code in PR #19464:
URL: https://github.com/apache/hudi/pull/19464#discussion_r3709592793
##########
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:
`TestClusteringUtils`: added the action assertion at the existing transition, plus `testTransitionInflightToCompleteReturnsCompletedReplaceCommit`, which calls `transitionClusteringOrReplaceInflightToComplete` directly for both inflight kinds and asserts `replacecommit` for each, so the `else` branch is covered too. It also asserts that the table-format hook is handed the same instant that is returned.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report completed timeline action in clustering callback [hudi]
Posted by "codope (via GitHub)" <gi...@apache.org>.
codope commented on code in PR #19464:
URL: https://github.com/apache/hudi/pull/19464#discussion_r3709600411
##########
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:
stubbed the inflight action.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report completed timeline action in clustering callback [hudi]
Posted by "codope (via GitHub)" <gi...@apache.org>.
codope commented on PR #19464:
URL: https://github.com/apache/hudi/pull/19464#issuecomment-5174839397
@voonhous Thanks for the review, all comments taken. On the two out-of-scope items, agreed on both. `ActiveTimelineV2:519` bypassing the metaserver override looks like a real bug of the same class, probably worth its own ticket.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report completed timeline action in clustering callback [hudi]
Posted by "codope (via GitHub)" <gi...@apache.org>.
codope commented on code in PR #19464:
URL: https://github.com/apache/hudi/pull/19464#discussion_r3721135172
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java:
##########
@@ -644,7 +645,7 @@ private void completeClustering(HoodieReplaceCommitMetadata replaceCommitMetadat
heartbeatClient.stop(clusteringCommitTime);
}
log.info("Clustering successfully on commit {} for table {}", clusteringCommitTime, table.getConfig().getBasePath());
- fireCommitCallbackIfNecessary(clusteringCommitTime, clusteringInstant.getAction(),
+ fireCommitCallbackIfNecessary(clusteringCommitTime, completedClusteringInstant.getAction(),
Review Comment:
done
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report completed timeline action in clustering callback [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19464:
URL: https://github.com/apache/hudi/pull/19464#issuecomment-5192392988
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15959",
"triggerID" : "4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28",
"triggerType" : "PUSH"
}, {
"hash" : "da413df0cc77c92c77978c121bb4f891714edb92",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16026",
"triggerID" : "da413df0cc77c92c77978c121bb4f891714edb92",
"triggerType" : "PUSH"
}, {
"hash" : "efab1ab82b36d8cdcb0f56e566c86f13d7661b81",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "efab1ab82b36d8cdcb0f56e566c86f13d7661b81",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* da413df0cc77c92c77978c121bb4f891714edb92 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16026)
* efab1ab82b36d8cdcb0f56e566c86f13d7661b81 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report completed timeline action in clustering callback [hudi]
Posted by "codope (via GitHub)" <gi...@apache.org>.
codope commented on code in PR #19464:
URL: https://github.com/apache/hudi/pull/19464#discussion_r3718134192
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java:
##########
@@ -644,7 +645,7 @@ private void completeClustering(HoodieReplaceCommitMetadata replaceCommitMetadat
heartbeatClient.stop(clusteringCommitTime);
}
log.info("Clustering successfully on commit {} for table {}", clusteringCommitTime, table.getConfig().getBasePath());
- fireCommitCallbackIfNecessary(clusteringCommitTime, clusteringInstant.getAction(),
+ fireCommitCallbackIfNecessary(clusteringCommitTime, completedClusteringInstant.getAction(),
Review Comment:
> for normal commits, the commit action type is inferred from `CommitUtils.getCommitActionType`
True for ingestion writes and the overwrite operation types, but it does not extend to table services. If you see `TestCommitUtils#testNormalOperationsDelegateBasedOnTableType` excludes only the four overwrite ops from its EnumSource, so it asserts this for CLUSTER and COMPACT today.
> we can just pass around the REPLACE_COMMIT_ACTION or just drop this param
It is a field on `HoodieWriteCommitCallbackMessage` (@PublicAPIClass, added by HUDI-7161), and the built-in `HoodieWriteCommitHttpCallback` serialises the message to JSON and POSTs it, a remote consumer has no timeline to filter. I would not prefer changing the public api. However, on just passing around REPLACE_COMMIT_ACTION, that does work. The only difference is copying the value versus reading the one the timeline just wrote. I preferred the latter so the mapping keeps a single definition, but I don't feel strongly. If you prefer I can switch to the constant and revert the util to return void. let me know.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report completed timeline action in clustering callback [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19464:
URL: https://github.com/apache/hudi/pull/19464#issuecomment-5193613931
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15959",
"triggerID" : "4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28",
"triggerType" : "PUSH"
}, {
"hash" : "da413df0cc77c92c77978c121bb4f891714edb92",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16026",
"triggerID" : "da413df0cc77c92c77978c121bb4f891714edb92",
"triggerType" : "PUSH"
}, {
"hash" : "efab1ab82b36d8cdcb0f56e566c86f13d7661b81",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16092",
"triggerID" : "efab1ab82b36d8cdcb0f56e566c86f13d7661b81",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* efab1ab82b36d8cdcb0f56e566c86f13d7661b81 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16092)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report completed timeline action in clustering callback [hudi]
Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #19464:
URL: https://github.com/apache/hudi/pull/19464#discussion_r3708642343
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java:
##########
@@ -644,7 +645,7 @@ private void completeClustering(HoodieReplaceCommitMetadata replaceCommitMetadat
heartbeatClient.stop(clusteringCommitTime);
}
log.info("Clustering successfully on commit {} for table {}", clusteringCommitTime, table.getConfig().getBasePath());
- fireCommitCallbackIfNecessary(clusteringCommitTime, clusteringInstant.getAction(),
+ fireCommitCallbackIfNecessary(clusteringCommitTime, completedClusteringInstant.getAction(),
Review Comment:
I don't see the reason why we need the action type for timeline instant filtering, the instant time is already passed in there and we know the instant is complete when calling the callback, the action type should just be a marker of what kidn of instant it was?
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report completed timeline action in clustering callback [hudi]
Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #19464:
URL: https://github.com/apache/hudi/pull/19464#discussion_r3710741723
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/BaseHoodieTableServiceClient.java:
##########
@@ -644,7 +645,7 @@ private void completeClustering(HoodieReplaceCommitMetadata replaceCommitMetadat
heartbeatClient.stop(clusteringCommitTime);
}
log.info("Clustering successfully on commit {} for table {}", clusteringCommitTime, table.getConfig().getBasePath());
- fireCommitCallbackIfNecessary(clusteringCommitTime, clusteringInstant.getAction(),
+ fireCommitCallbackIfNecessary(clusteringCommitTime, completedClusteringInstant.getAction(),
Review Comment:
so why we even pass around the commit action type then? if the timeline filtering is always there. for normal commits, the commit action type is inferred from `CommitUtils.getCommitActionType` which does not switch between different instant state, we can just pass around the REPLACE_COMMIT_ACTION or just drop this param?
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(client): report completed timeline action in clustering callback [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19464:
URL: https://github.com/apache/hudi/pull/19464#issuecomment-5174795556
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15959",
"triggerID" : "4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28",
"triggerType" : "PUSH"
}, {
"hash" : "da413df0cc77c92c77978c121bb4f891714edb92",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "da413df0cc77c92c77978c121bb4f891714edb92",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 4b28420c5b2b1a584bfe7d4027e57c8ecffaeb28 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=15959)
* da413df0cc77c92c77978c121bb4f891714edb92 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org