wombatu-kun commented on code in PR #19444:
URL: https://github.com/apache/hudi/pull/19444#discussion_r3697383690
##########
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:
`WriteMarkersFactory` still falls back to direct markers on HDFS, and
accepts a remote view storage type in place of an embedded server, so a running
embedded timeline server is neither necessary nor sufficient for this to hold.
Either drop that clause or mirror the actual gate in `WriteMarkersFactory`.
##########
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:
`HoodieWriteConfig.Builder` defaults `engineType` to SPARK and
`HoodieJavaWriteClient` never checks it, so a Java-client config built without
`withEngineType(EngineType.JAVA)` resolves to `TIMELINE_SERVER_BASED` with
nothing set explicitly. Scope the claim to a config that sets `EngineType.JAVA`
rather than to the Java write client generally.
##########
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:
Both parameter cases run identical assertions, so nothing here would notice
if `WriteMarkersFactory` fell back to `DirectWriteMarkers` and the
`TIMELINE_SERVER_BASED` case stopped covering the timeline-server path. Branch
on `markerType` and check the instant's marker folder before the commit deletes
it - only the timeline-server path writes `MARKERS.type` - as
`TestWriteCopyOnWrite#testMarkType` does on the Flink side.
##########
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:
`hoodie.embed.timeline.server` already defaults to true, so this line
changes nothing and `testInsert` in this class already runs the Java client
against an embedded timeline server. What is missing is
`withEnableBackupForRemoteFileSystemView(false)`, which
`HoodieJavaClientTestHarness#getConfigBuilder` sets precisely so a
timeline-server failure fails the test instead of silently falling back to a
local file-system view.
--
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]
