wombatu-kun commented on code in PR #19369:
URL: https://github.com/apache/hudi/pull/19369#discussion_r3643043796
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/marker/TimelineServerBasedWriteMarkers.java:
##########
@@ -137,7 +139,7 @@ protected Option<StoragePath> create(String partitionPath,
String fileName, IOTy
if (success) {
return Option.of(new
StoragePath(FSUtils.constructAbsolutePath(markerDirPath, partitionPath),
markerFileName));
} else {
- return Option.empty();
+ throw new HoodieIOException("[timeline-server-based] Failed to create
marker for partition " + partitionPath + ", fileName " + fileName + " with
IOType " + type);
Review Comment:
A false result here does not mean the create failed: the server returns
false when the marker already exists (MarkerDirState sets isSuccessful to
!exists), and a genuine I/O failure already surfaces as HoodieRemoteException,
not as false. So this throw fires on an already-existing marker, which
createIfNotExists/createLogMarkerIfNotExists treat as a harmless Option.empty
no-op - is turning already-exists into a hard failure intended, or should the
throw be limited to a real create failure?
##########
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:
Returning false here mirrors a create failure, but the real server also
returns false when the marker already exists (MarkerDirState: !exists), so the
test cannot tell a genuine failure from a benign already-exists and never
covers createIfNotExists on an existing marker. Add a case that creates the
same marker twice to lock down the intended behavior.
##########
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 {
Review Comment:
@AfterEach cleanup() already closes timelineService, so closing it again in
this finally double-closes the service. Drop the finally - the try only needs
to wrap the failing create.
##########
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) {
Review Comment:
This MockTimelineServerBasedWriteMarkers(HoodieTable, String) constructor is
unused - the test builds the mock through the (String, String, String,
FileSystemViewStorageConfig) constructor. Remove it.
--
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]
