wombatu-kun commented on code in PR #20409:
URL: https://github.com/apache/hudi/pull/20409#discussion_r4355637189
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/timeline/TimelineArchiverV1.java:
##########
@@ -150,9 +152,24 @@ public int archiveIfRequired(HoodieEngineContext context,
boolean acquireLock) t
addArchivalCommitMetrics(instantsToArchive);
boolean success = true;
if (!instantsToArchive.isEmpty()) {
- this.writer = openWriter(archiveFilePath.getParent());
log.info("Archiving instants {} for table {}", instantsToArchive,
config.getBasePath());
- archive(context, instantsToArchive);
+ try {
+ this.writer = openWriter(archiveFilePath.getParent());
+ archive(context, instantsToArchive);
+ close();
+ } catch (Exception e) {
+ try {
+ close();
+ } catch (Exception closeException) {
+ e.addSuppressed(closeException);
+ }
+ if (this.config.isFailOnTimelineArchivingEnabled()) {
+ throw e;
+ }
+ recordArchivalFailure(e);
+ log.warn("Failed to archive instants for table {}; active timeline
files will not be deleted", config.getBasePath(), e);
+ return 0;
Review Comment:
With the flag false, a V1 instant whose file is non-empty but unparseable
now fails this batch on every attempt, so archival never gets past it and the
active timeline grows without bound, with only a WARN and `archivalStatus=-1`
to show for it. Is that the intended trade-off, or should a permanently
unparseable instant still have a way through, e.g. archived as a placeholder
the way an empty instant already is?
##########
hudi-client/hudi-client-common/src/test/java/org/apache/hudi/common/table/timeline/TestArchivedTimelineV2.java:
##########
@@ -135,10 +139,53 @@ void getInstantReaderReferencesSelf() {
HoodieArchivedTimeline timeline =
TIMELINE_FACTORY.createArchivedTimeline(metaClient);
assertSame(timeline, timeline.getInstantReader());
}
+
+ @Test
+ void testFailedActionDoesNotPublishPartialArchiveFile() throws Exception {
+ ActiveAction firstAction = createActiveAction("10000001");
+ ActiveAction failedAction = new DummyActiveAction(
+ INSTANT_GENERATOR.createNewInstant(HoodieInstant.State.COMPLETED,
HoodieTimeline.COMMIT_ACTION, "10000002", "10000102"),
+ new byte[0]) {
+ @Override
+ public Option<byte[]> getCommitMetadata(HoodieTableMetaClient
metaClient) {
+ throw new IllegalStateException("injected archive failure");
+ }
+ };
+ ActiveAction lastAction = createActiveAction("10000003");
+ LSMTimelineWriter writer = createTimelineWriter();
+
+ assertThrows(HoodieCommitException.class,
+ () -> writer.write(Arrays.asList(firstAction, failedAction,
lastAction), Option.empty()));
+ assertTrue(metaClient.getArchivedTimeline().empty());
+ assertEquals(1, LSMTimeline.listAllMetaFiles(metaClient,
metaClient.getArchivePath()).size());
+
+ writer.write(Arrays.asList(firstAction, createActiveAction("10000002"),
lastAction), Option.empty());
+ assertEquals(3, metaClient.getArchivedTimeline().reload().countInstants());
+ }
+
+ @Test
+ void testCommittedArchiveFileMustContainAllExpectedActions() {
+ ActiveAction firstAction = createActiveAction("10000001");
+ ActiveAction middleAction = createActiveAction("10000002");
+ ActiveAction lastAction = createActiveAction("10000003");
+ LSMTimelineWriter writer = createTimelineWriter();
+
+ writer.write(Arrays.asList(firstAction, lastAction), Option.empty());
Review Comment:
Only the rejecting branch of `containsAllActions` is covered, so a check
that always returned false would still pass here while failing every legitimate
retry of a committed batch. Could this test also re-write `[firstAction,
lastAction]` right after this line and assert it returns without throwing?
--
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]
