You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@hudi.apache.org by "rangareddy (via GitHub)" <gi...@apache.org> on 2026/07/31 10:53:51 UTC

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

rangareddy opened a new pull request, #19444:
URL: https://github.com/apache/hudi/pull/19444

### Describe the issue this Pull Request addresses

Closes #15478 (HUDI-5011).

The ticket asks for the Java write client to be exercised against the embedded timeline server, after a
community report, with DIRECT markers given as the interim workaround. It carries no reproduction and no
detail beyond that.

Nothing covered the combination. `HoodieJavaClientTestHarness`, `TestJavaHoodieBackedMetadata`,
`TestHoodieMetadataBase` and `HoodieFileGroupReaderOnJavaTestBase` all call
`withEmbeddedTimelineServerEnabled(false)`, and the one test that parameterises it,
`TestHoodieJavaWriteClientInsert#testWriteClientAndTableServiceClientWithTimelineServer`, only checks that
the write client and table-service client share a passed-in `EmbeddedTimelineService` — it performs no write
and never touches markers. `hudi-java-client` had no reference to `WriteMarkersFactory` at all.

The marker angle matters because `HoodieWriteConfig.Builder#getDefaultMarkersType` returns `DIRECT` for the
Java engine, so the workaround suggested on the ticket is already the default; the timeline-server-based path
is only reached when `hoodie.write.markers.type` is set explicitly.

### Summary and Changelog

- `TestHoodieJavaWriteClientInsert#testInsertWithEmbeddedTimelineServerAndMarkerType`: parameterised over
both `MarkerType` values with `withEmbeddedTimelineServerEnabled(true)`. It asserts the embedded timeline
server is present, performs a real insert and commit, and checks the commit completed and a base file was
written.

Both cases pass. The `TIMELINE_SERVER_BASED` case genuinely instantiates
`TimelineServerBasedWriteMarkers` (confirmed from `WriteMarkersFactory` debug output, with no "Falling back
to direct markers" warning), so the timeline-server marker path runs end to end against the embedded
server. The reported problem does not reproduce on current master with either marker type.

- `HoodieWriteConfig#getDefaultMarkersType`: corrected the comment. It read "Timeline-server-based marker is
not supported for Flink and Java engines", which is not accurate —
`TestFlinkWriteClients#testMarkerType` already asserts Flink receives `TimelineServerBasedWriteMarkers`
when the type is set explicitly, and the new test shows the same for Java. `DIRECT` is the *default* for
those engines, not the only option. Comment only, no behaviour change.

Worth mentioning what I did not do: I first implemented an extra fallback in `WriteMarkersFactory.get` so
that `JAVA`/`FLINK` asking for `TIMELINE_SERVER_BASED` would warn and fall back to `DIRECT`, matching the two
fallbacks already there and the comment above. `TestFlinkWriteClients#testMarkerType` asserts the opposite,
so that change would have broken it and removed a working capability rather than fixing a bug. It was
reverted, and the misleading comment corrected instead.

### Impact

Test coverage plus a comment. No production behaviour change, no API, config or format change.

### Risk Level

none

### Documentation Update

none — no new config and no default value change.

### Contributor's checklist

- [x] Read through [contributor's guide](https://hudi.apache.org/contribute/how-to-contribute)
- [x] Enough context is provided in the sections above
- [x] Adequate tests were added if applicable
- [x] CI passes on my PR

--
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

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

Posted by "rangareddy (via GitHub)" <gi...@apache.org>.

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

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

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

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.

codecov-commenter commented on PR #19444:
URL: https://github.com/apache/hudi/pull/19444#issuecomment-5142320102

   ## [Codecov](https://app.codecov.io/gh/apache/hudi/pull/19444?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   :white_check_mark: All modified and coverable lines are covered by tests.
   :white_check_mark: Project coverage is 70.99%. Comparing base ([`f9c19c0`](https://app.codecov.io/gh/apache/hudi/commit/f9c19c0c5b38b361bb1d307791cf70eff92b61ee?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5651c55`](https://app.codecov.io/gh/apache/hudi/commit/5651c5527e015f7f9402ac496f814a2a04df0947?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 1 commits behind head on master.
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #19444      +/-   ##
   ============================================
   - Coverage     73.08%   70.99%   -2.09%     
   + Complexity    31620    30923     -697     
   ============================================
     Files          2576     2576              
     Lines        143558   143561       +3     
     Branches      17607    17607              
   ============================================
   - Hits         104921   101924    -2997     
   - Misses        30615    34061    +3446     
   + Partials       8022     7576     -446     
   ```
   
   | [Components](https://app.codecov.io/gh/apache/hudi/pull/19444/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [hudi-common](https://app.codecov.io/gh/apache/hudi/pull/19444/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.52% <ø> (+0.33%)` | :arrow_up: |
   | [hudi-client](https://app.codecov.io/gh/apache/hudi/pull/19444/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `80.62% <ø> (+2.18%)` | :arrow_up: |
   | [hudi-flink](https://app.codecov.io/gh/apache/hudi/pull/19444/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `83.97% <ø> (ø)` | |
   | [hudi-spark-datasource](https://app.codecov.io/gh/apache/hudi/pull/19444/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.86% <ø> (-13.32%)` | :arrow_down: |
   | [hudi-utilities](https://app.codecov.io/gh/apache/hudi/pull/19444/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `68.66% <ø> (-3.53%)` | :arrow_down: |
   | [hudi-cli](https://app.codecov.io/gh/apache/hudi/pull/19444/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `15.32% <ø> (ø)` | |
   | [hudi-hadoop](https://app.codecov.io/gh/apache/hudi/pull/19444/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `62.80% <ø> (+0.69%)` | :arrow_up: |
   | [hudi-sync](https://app.codecov.io/gh/apache/hudi/pull/19444/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.42% <ø> (-0.48%)` | :arrow_down: |
   | [hudi-io](https://app.codecov.io/gh/apache/hudi/pull/19444/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `79.55% <ø> (+0.09%)` | :arrow_up: |
   | [hudi-timeline-service](https://app.codecov.io/gh/apache/hudi/pull/19444/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `84.23% <ø> (+0.78%)` | :arrow_up: |
   | [hudi-cloud](https://app.codecov.io/gh/apache/hudi/pull/19444/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `64.00% <ø> (ø)` | |
   | [hudi-kafka-connect](https://app.codecov.io/gh/apache/hudi/pull/19444/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.20% <ø> (ø)` | |
   
   | [Flag](https://app.codecov.io/gh/apache/hudi/pull/19444/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [hadoop-mr-java-client](https://app.codecov.io/gh/apache/hudi/pull/19444/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `43.76% <ø> (+0.34%)` | :arrow_up: |
   | [spark-client-hadoop-common](https://app.codecov.io/gh/apache/hudi/pull/19444/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `48.68% <ø> (+<0.01%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/hudi/pull/19444?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...java/org/apache/hudi/config/HoodieWriteConfig.java](https://app.codecov.io/gh/apache/hudi/pull/19444?src=pr&el=tree&filepath=hudi-client%2Fhudi-client-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Fconfig%2FHoodieWriteConfig.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1jbGllbnQvaHVkaS1jbGllbnQtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWRpL2NvbmZpZy9Ib29kaWVXcml0ZUNvbmZpZy5qYXZh) | `91.77% <ø> (+1.19%)` | :arrow_up: |
   
   ... and [474 files with indirect coverage changes](https://app.codecov.io/gh/apache/hudi/pull/19444/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   </details>
   <details><summary> :rocket: New features to boost your workflow: </summary>
   
   - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
   - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
   </details>


-- 
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