rangareddy commented on code in PR #19444:
URL: https://github.com/apache/hudi/pull/19444#discussion_r3701455264
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java:
##########
@@ -3918,7 +3918,8 @@ private String getDefaultMarkersType(EngineType
engineType) {
}
case FLINK:
case JAVA:
- // Timeline-server-based marker is not supported for Flink and Java
engines
+ // Timeline-server-based markers are not the default for Flink and
Java, but they do work when
+ // hoodie.write.markers.type is set explicitly and an embedded
timeline server is running.
Review Comment:
Corrected in the latest commit. The comment now says timeline-server markers
are selected by setting `hoodie.write.markers.type` explicitly, "subject to the
same gates `WriteMarkersFactory` applies to every engine", rather than claiming
a running embedded server is the condition. You are right that it is neither
necessary nor sufficient — the HDFS fallback and the `isRemoteViewStorageType`
branch both cut across it.
##########
hudi-client/hudi-java-client/src/test/java/org/apache/hudi/client/TestHoodieJavaWriteClientInsert.java:
##########
@@ -140,6 +142,39 @@ public void
testWriteClientAndTableServiceClientWithTimelineServer(
writeClient.close();
}
+ /**
+ * HUDI-5011: exercises the Java write client against both marker types with
the embedded timeline
+ * server running. {@code HoodieWriteConfig.Builder} defaults the Java
engine to
+ * {@link MarkerType#DIRECT}, so the timeline-server-based path is only
reached when
Review Comment:
Scoped as suggested. The javadoc now says the default applies to the
config's engine type rather than to the client, and notes explicitly that
`HoodieJavaWriteClient` never inspects it, so a config left on the builder's
SPARK default resolves to TIMELINE_SERVER_BASED on its own.
`makeHoodieClientConfigBuilder` in this class does set
`withEngineType(EngineType.JAVA)`, so the test itself was on the right side of
that, but the claim was broader than the setup.
##########
hudi-client/hudi-java-client/src/test/java/org/apache/hudi/client/TestHoodieJavaWriteClientInsert.java:
##########
@@ -140,6 +142,39 @@ public void
testWriteClientAndTableServiceClientWithTimelineServer(
writeClient.close();
}
+ /**
+ * HUDI-5011: exercises the Java write client against both marker types with
the embedded timeline
+ * server running. {@code HoodieWriteConfig.Builder} defaults the Java
engine to
+ * {@link MarkerType#DIRECT}, so the timeline-server-based path is only
reached when
+ * {@code hoodie.write.markers.type} is set explicitly, and nothing covered
that combination.
+ */
+ @ParameterizedTest
+ @EnumSource(MarkerType.class)
+ public void testInsertWithEmbeddedTimelineServerAndMarkerType(MarkerType
markerType) throws Exception {
+ HoodieWriteConfig config = makeHoodieClientConfigBuilder(basePath)
+ .withEmbeddedTimelineServerEnabled(true)
Review Comment:
Both applied. Dropped `withEmbeddedTimelineServerEnabled(true)` since it
only restated the default, and added
`withEnableBackupForRemoteFileSystemView(false)` so a timeline-server failure
fails the test rather than silently falling back to a local view.
##########
hudi-client/hudi-java-client/src/test/java/org/apache/hudi/client/TestHoodieJavaWriteClientInsert.java:
##########
@@ -140,6 +142,39 @@ public void
testWriteClientAndTableServiceClientWithTimelineServer(
writeClient.close();
}
+ /**
+ * HUDI-5011: exercises the Java write client against both marker types with
the embedded timeline
+ * server running. {@code HoodieWriteConfig.Builder} defaults the Java
engine to
+ * {@link MarkerType#DIRECT}, so the timeline-server-based path is only
reached when
+ * {@code hoodie.write.markers.type} is set explicitly, and nothing covered
that combination.
+ */
+ @ParameterizedTest
+ @EnumSource(MarkerType.class)
+ public void testInsertWithEmbeddedTimelineServerAndMarkerType(MarkerType
markerType) throws Exception {
+ HoodieWriteConfig config = makeHoodieClientConfigBuilder(basePath)
+ .withEmbeddedTimelineServerEnabled(true)
+ .withMarkersType(markerType.name())
Review Comment:
Good catch, and this was the weakest part of the test. It now reads the
marker directory before the commit removes it and branches on the type:
`MarkerUtils.doesMarkerTypeFileExist` must be true for TIMELINE_SERVER_BASED
and false for DIRECT, since only the timeline-server path writes `MARKERS.type`.
Checked it actually discriminates rather than just passing: forcing DIRECT
while the parameter said TIMELINE_SERVER_BASED fails with
"Timeline-server-based markers should have written MARKERS.type", which is
exactly the fallback you described.
--
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]
