hudi-agent commented on code in PR #19661:
URL: https://github.com/apache/hudi/pull/19661#discussion_r3806535059
##########
hudi-spark-datasource/hudi-spark/src/test/java/org/apache/hudi/table/action/rollback/TestCopyOnWriteRollbackActionExecutor.java:
##########
@@ -601,7 +601,7 @@ public void testRollbackWhenReplaceCommitIsPresent() throws
Exception {
String rollbackInstant = WriteClientTestUtils.createNewInstantTime();
BaseRollbackPlanActionExecutor copyOnWriteRollbackPlanActionExecutor =
new BaseRollbackPlanActionExecutor(context, table.getConfig(), table,
rollbackInstant, needRollBackInstant, false,
- !table.getConfig().shouldRollbackUsingMarkers(), false);
+ table.getConfig().shouldRollbackUsingMarkers(), false,
Option.empty());
Review Comment:
🤖 Besides appending `Option.empty()`, this line also drops the `!` on
`shouldRollbackUsingMarkers()` — was that intentional? Every other call site in
this PR keeps its original boolean, so it stands out. It happens to be a no-op
here since `needRollBackInstant` is completed (the constructor forces
marker-based off for completed instants), but wanted to confirm it isn't an
accidental find/replace artifact.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/client/TestCommitMetadataProperties.java:
##########
@@ -155,6 +164,120 @@ void enrich_preservesCallerProvidedKeys() {
assertNotNull(result.get(HUDI_VERSION_KEY));
}
+ // ---- Tests for extraMetadata in all instant types ----
+
+ @Test
+ void rollbackPlan_extraMetadataFieldPresent() {
+ Map<String, String> extraMetadata = new HashMap<>();
+ extraMetadata.put(HUDI_VERSION_KEY, "1.2.0");
+ extraMetadata.put(ENGINE_KEY, "SPARK");
+
+ HoodieRollbackPlan plan = new HoodieRollbackPlan(
+ new HoodieInstantInfo("001", "commit"),
+ Collections.emptyList(), 1, extraMetadata);
+
+ assertEquals("1.2.0", plan.getExtraMetadata().get(HUDI_VERSION_KEY));
+ assertEquals("SPARK", plan.getExtraMetadata().get(ENGINE_KEY));
+ }
+
+ @Test
+ void rollbackPlan_extraMetadataNullByDefault() {
+ HoodieRollbackPlan plan = new HoodieRollbackPlan(
+ new HoodieInstantInfo("001", "commit"),
+ Collections.emptyList(), 1, null);
Review Comment:
🤖 nit: `assertEquals(null, ...)` reads a bit awkwardly —
`assertNull(plan.getExtraMetadata())` would express the intent more directly.
Same pattern applies to the other `assertEquals(null, ...)` calls in this file.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/table/TestBaseHoodieTable.java:
##########
@@ -130,7 +130,8 @@ public HoodieCleanMetadata clean(HoodieEngineContext
context, String cleanInstan
@Override
public Option<HoodieRollbackPlan> scheduleRollback(HoodieEngineContext
context, String instantTime, HoodieInstant instantToRollback,
- boolean
skipTimelinePublish, boolean shouldRollbackUsingMarkers, boolean isRestore) {
+ boolean
skipTimelinePublish, boolean shouldRollbackUsingMarkers, boolean isRestore,
+ Option extraMetadata) {
countOfScheduleRollbackFunctionCalls++;
Review Comment:
🤖 nit: could you parameterize the raw `Option` here to `Option<Map<String,
String>>`? The production abstract method uses the full generic type, so
leaving it raw in the stub is inconsistent and will produce an unchecked-cast
warning.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/restore/CopyOnWriteRestoreActionExecutor.java:
##########
@@ -53,7 +53,7 @@ protected HoodieRollbackMetadata
rollbackInstant(HoodieInstant instantToRollback
transactionManager.beginStateChange(Option.empty(), Option.empty());
try {
newInstantTime = table.getMetaClient().createNewInstantTime(false);
- table.scheduleRollback(context, newInstantTime, instantToRollback,
false, false, true);
+ table.scheduleRollback(context, newInstantTime, instantToRollback,
false, false, true, Option.empty());
Review Comment:
🤖 These restore-internal rollbacks (and the rollbackInflightInstant /
rollbackInflightLogCompaction paths in HoodieTable) still pass Option.empty(),
so the rollback instants they produce won't carry the hudi.version/engine debug
metadata. Is that intentional, or worth enriching too given the "all instant
types" goal? The user-facing rollback path in resolveOrScheduleRollback does
enrich.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
