voonhous commented on code in PR #19369:
URL: https://github.com/apache/hudi/pull/19369#discussion_r3802702280
##########
hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/table/marker/TestTimelineServerBasedWriteMarkers.java:
##########
@@ -162,6 +168,54 @@ private void restartServerAndClient(int
numberOfSimulatedConnectionFailures,
}
}
+ @Test
+ public void testMarkerCreationFailure() throws IOException {
+ FileSystemViewStorageConfig.Builder builder =
FileSystemViewStorageConfig.newBuilder().withRemoteServerHost("localhost")
+ .withRemoteServerPort(timelineService.getServerPort())
+ .withRemoteTimelineClientTimeoutSecs(DEFAULT_READ_TIMEOUT_SECS);
+ MockTimelineServerBasedWriteMarkers timelineServerBasedWriteMarkers = new
MockTimelineServerBasedWriteMarkers(basePath, markerFolderPath.toString(),
"000", builder.build());
+ // this should succeed.
+ timelineServerBasedWriteMarkers.create("2020/06/01", "file1",
IOType.MERGE);
+
+ assertTrue(storage.exists(markerFolderPath));
+ assertTrue(writeMarkers.doesMarkerDirExist());
+
+ // lets fail the marker creation
+ timelineServerBasedWriteMarkers.failMarkerCreation = true;
+ try {
+ timelineServerBasedWriteMarkers.create("2020/06/01", "file2",
IOType.MERGE);
+ fail("Should not have reached here");
+ } catch (HoodieIOException ioe) {
+ assertTrue(ioe.getMessage().contains("[timeline-server-based] Failed to
create marker for partition"));
+ } finally {
+ if (timelineService != null) {
+ timelineService.close();
+ }
+ }
+ }
+
+ static class MockTimelineServerBasedWriteMarkers extends
TimelineServerBasedWriteMarkers {
+
+ boolean failMarkerCreation = false;
+
+ public MockTimelineServerBasedWriteMarkers(HoodieTable table, String
instantTime) {
+ super(table, instantTime);
+ }
+
+ MockTimelineServerBasedWriteMarkers(String basePath, String
markerFolderPath, String instantTime, FileSystemViewStorageConfig
fileSystemViewStorageConfig) {
+ super(basePath, markerFolderPath, instantTime,
fileSystemViewStorageConfig);
+ }
+
+ @Override
+ boolean executeCreateMarkerRequest(Map<String, String> paramsMap, String
partitionPath, String markerFileName) {
+ if (!failMarkerCreation) {
+ return super.executeCreateMarkerRequest(paramsMap, partitionPath,
markerFileName);
+ } else {
+ return false;
Review Comment:
+1, and it is a bit stronger than "cannot tell them apart":
`MockTimelineServerBasedWriteMarkers` returns `false` before ever calling
`super`, so no request reaches the server. The test asserts "if this method
returns false, we throw", which restates the diff rather than exercising
anything -- it would still pass if the client and server disagreed completely
about what `false` means.
Two concrete changes:
1. Drop the mock for this case and call `create("2020/06/01", "file1",
IOType.MERGE)` twice against the real `timelineService` already started in
`setup()`, asserting the second call's outcome. That covers client -> server ->
already-exists -> client for real.
2. Put the already-exists contract in `TestWriteMarkersBase` rather than
here. Nothing pins it today: `createSomeMarkers` creates each marker exactly
once, and `createLogMarkerIfNotExists` is called once on a single file. A test
there runs against `DirectWriteMarkers`, `DirectWriteMarkersV1`,
`TimelineServerBasedWriteMarkers` and `TimelineServerBasedWriteMarkersV1`, so
it would settle whether the two implementations are meant to agree -- which is
the open question in this thread.
--
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]
