rangareddy commented on code in PR #19444:
URL: https://github.com/apache/hudi/pull/19444#discussion_r3701710379
##########
hudi-client/hudi-java-client/src/test/java/org/apache/hudi/client/TestHoodieJavaWriteClientInsert.java:
##########
@@ -140,6 +144,61 @@ public void
testWriteClientAndTableServiceClientWithTimelineServer(
writeClient.close();
}
+ /**
+ * HUDI-5011: exercises a Java-engine write config against both marker types
with the embedded timeline
+ * server. {@code HoodieWriteConfig.Builder} defaults {@link
MarkerType#DIRECT} for
+ * {@link EngineType#JAVA}, so the timeline-server-based path is only
reached when
+ * {@code hoodie.write.markers.type} is set explicitly. Note the default
applies to the config's engine
+ * type, not to the client: {@code HoodieJavaWriteClient} never inspects it,
so a config left on the
+ * builder's SPARK default would resolve to TIMELINE_SERVER_BASED on its own.
+ *
+ * <p>Backup for the remote file system view is disabled so that a
timeline-server failure fails the test
+ * rather than silently falling back to a local view, matching
+ * {@code HoodieJavaClientTestHarness#getConfigBuilder}.
+ */
+ @ParameterizedTest
+ @EnumSource(MarkerType.class)
+ public void testInsertWithEmbeddedTimelineServerAndMarkerType(MarkerType
markerType) throws Exception {
+ HoodieWriteConfig config = makeHoodieClientConfigBuilder(basePath)
+ .withMarkersType(markerType.name())
+ .withFileSystemViewConfig(FileSystemViewStorageConfig.newBuilder()
+ .withEnableBackupForRemoteFileSystemView(false).build())
+ .build();
+
+ HoodieJavaWriteClient writeClient = getHoodieWriteClient(config);
+ assertTrue(writeClient.getTimelineServer().isPresent(),
+ "The embedded timeline server should be running for marker type " +
markerType);
+
+ List<HoodieRecord> records = new ArrayList<>();
+ records.add(createSimpleRecord("1", "2021-09-11T16:16:41.415Z", 1));
+ records.add(createSimpleRecord("2", "2021-09-11T16:16:41.415Z", 2));
+
+ String commitTime = makeNewCommitTime(1, "%09d");
+ WriteClientTestUtils.startCommitWithTime(writeClient, commitTime);
+ List<WriteStatus> statuses = writeClient.insert(records, commitTime);
+
+ // Inspect the markers before commit removes them. Only the
timeline-server path writes MARKERS.type,
+ // so this is what would notice a silent fallback to DirectWriteMarkers.
+ metaClient = HoodieTableMetaClient.reload(metaClient);
+ StoragePath markerDir = new
StoragePath(metaClient.getMarkerFolderPath(commitTime));
+ boolean markerTypeFileExists =
MarkerUtils.doesMarkerTypeFileExist(metaClient.getStorage(), markerDir);
+ if (markerType == MarkerType.TIMELINE_SERVER_BASED) {
+ assertTrue(markerTypeFileExists,
+ "Timeline-server-based markers should have written " +
MarkerUtils.MARKER_TYPE_FILENAME);
+ } else {
+ assertFalse(markerTypeFileExists,
Review Comment:
Fixed in `ecc646a` rather than deferred — you are right that the DIRECT arm
was passing on an absence that a no-marker write would satisfy just as well.
Each arm now also requires the artifact its own path is supposed to produce,
read recursively off storage rather than through the timeline server so the
assertion does not lean on the path it is checking:
```java
List<String> markerFileNames = listMarkerFileNames(markerDir);
if (markerType == MarkerType.TIMELINE_SERVER_BASED) {
assertTrue(markerTypeFileExists, ...);
assertTrue(markerFileNames.stream().anyMatch(
name -> name.startsWith(MarkerUtils.MARKERS_FILENAME_PREFIX)
&& !name.equals(MarkerUtils.MARKER_TYPE_FILENAME)), ...);
} else {
assertFalse(markerTypeFileExists, ...);
// Absence of MARKERS.type alone would also hold if the write produced no
markers at all, so
// require the direct markers themselves.
assertTrue(markerFileNames.stream().anyMatch(name ->
name.contains(HoodieTableMetaClient.MARKER_EXTN)), ...);
}
```
What each arm actually writes for this insert, which is what the assertions
are pinned to:
```
DIRECT ->
[d51e29b8-...-0_0-0-0_000000001.parquet.marker.CREATE]
TIMELINE_SERVER_BASED -> [MARKERS.type, MARKERS0]
```
Checked both new assertions independently rather than assuming they bite.
Pointing `markerDir` at an instant with no marker directory — the "wrote
nothing" case you described — turns the DIRECT arm red, where before it passed:
```
AssertionFailedError: Direct markers should have written a .marker file.
Found: []
```
And with the pre-existing `MARKERS.type` assertion removed so the new one is
the only thing under test, the timeline-server arm fails the same way:
```
AssertionFailedError: Timeline-server-based markers should have written a
MARKERS<n> file. Found: []
```
Whole class green with the fix in place: `Tests run: 9, Failures: 0, Errors:
0`, checkstyle and `apache-rat:check` clean.
--
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]
