Re: [PR] test(java-client): cover both marker types with the embedded timeline server [hudi]

["wombatu-kun (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org