wombatu-kun commented on code in PR #19811:
URL: https://github.com/apache/hudi/pull/19811#discussion_r3953797742


##########
hudi-utilities/src/test/java/org/apache/hudi/utilities/deltastreamer/TestHoodieMultiTableDeltaStreamer.java:
##########
@@ -243,6 +247,109 @@ public void testMultiTableExecutionWithParquetSource() 
throws IOException {
     }
   }
 
+  @Test
+  public void testFailFastOnContinuousDefaultsToFalse() {
+    HoodieMultiTableDeltaStreamer.Config cfg = new 
HoodieMultiTableDeltaStreamer.Config();
+    assertFalse(cfg.failFastOnContinuousMode);
+  }
+
+  @Timeout(600)
+  @Test
+  public void testMultiTableContinuousModeSyncsAllTablesInParallel() throws 
IOException {
+    HoodieMultiTableDeltaStreamer streamer = 
setupContinuousStreamer("parquetContinuous", false);
+    List<TableExecutionContext> contexts = 
streamer.getTableExecutionContexts();
+    // Let each table stop on its own once it has ingested its data, so the 
test does not run forever.
+    setTerminationStrategy(contexts);
+
+    streamer.sync();
+
+    assertEquals(2, streamer.getSuccessTables().size());
+    assertTrue(streamer.getFailedTables().isEmpty());
+    assertRecordCount(10, contexts.get(0).getConfig().targetBasePath, 
sqlContext);
+    assertRecordCount(5, contexts.get(1).getConfig().targetBasePath, 
sqlContext);
+  }
+
+  @Timeout(600)
+  @Test
+  public void testFailFastOnContinuousThrowsWhenATableFails() throws 
IOException {
+    HoodieMultiTableDeltaStreamer streamer = 
setupContinuousStreamer("parquetFailFast", true);
+    List<TableExecutionContext> contexts = 
streamer.getTableExecutionContexts();
+    // Table 1 blocks after the barrier, so only fail fast interrupting it can 
end its sync. Table 2 fails after the
+    // barrier. This proves fail fast tears down a sibling that is still 
actively running, not one that stopped itself.
+    
contexts.get(0).getProperties().setProperty(ContinuousTestSource.BLOCK_UNTIL_INTERRUPTED,
 "true");
+    
contexts.get(1).getProperties().setProperty(ContinuousTestSource.FAIL_AFTER_BARRIER,
 "true");
+
+    assertThrows(HoodieException.class, streamer::sync);
+    assertFalse(streamer.getFailedTables().isEmpty());
+    // sync() returns only after the blocked sibling was interrupted, so the 
latch must already be counted down.
+    assertTrue(ContinuousTestSource.wasBlockedTableInterrupted());
+  }
+
+  @Timeout(600)
+  @Test
+  public void testFailFastOnContinuousAfterASiblingFinished() throws 
IOException {

Review Comment:
   Both fail-fast tests end in awaitFailFast's catch, so the branch where every 
table terminates normally under fail fast is never exercised. Worth adding a 
case with failFastOnContinuousMode on and NoNewDataTerminationStrategy on both 
tables, asserting sync() returns and both tables land in successTables.



##########
hudi-utilities/src/main/java/org/apache/hudi/utilities/streamer/HoodieMultiTableStreamer.java:
##########
@@ -461,28 +484,194 @@ private static String resetTarget(Config configuration, 
String database, String
 
   /**
    * Creates actual HoodieDeltaStreamer objects for every table/topic and does 
incremental sync.
+   *
+   * <p>In continuous mode each table's sync blocks until it is shut down, so 
the tables are synced concurrently.
+   * Otherwise the tables are synced sequentially, one after another.
    */
   public void sync() {
+    try {
+      if (continuousMode) {
+        syncContinuously();
+      } else {
+        syncSequentially();
+      }
+    } finally {
+      log.info("Ingestion was successful for topics: {}", successTables);
+      if (!failedTables.isEmpty()) {
+        log.info("Ingestion failed for topics: {}", failedTables);
+      }
+    }
+  }
+
+  private void syncSequentially() {
     for (TableExecutionContext context : tableExecutionContexts) {
       HoodieStreamer streamer = null;
       try {
         streamer = new HoodieStreamer(context.getConfig(), jssc, 
Option.ofNullable(context.getProperties()));
         streamer.sync();
         successTables.add(Helpers.getTableWithDatabase(context));
-        streamer.shutdownGracefully();
       } catch (Exception e) {
         log.error("error while running MultiTableDeltaStreamer for table: {}", 
context.getTableName(), e);
         failedTables.add(Helpers.getTableWithDatabase(context));
       } finally {
         if (streamer != null) {
-          streamer.shutdownGracefully();
+          shutdownQuietly(streamer, context);
         }
       }
     }
+  }
+
+  /**
+   * Syncs all tables concurrently, one thread per table. Used for continuous 
mode where each table's sync blocks
+   * indefinitely.
+   *
+   * <p>When {@code --fail-fast-on-continuous} is enabled, the first table 
failure fails the whole job. The sibling
+   * streamers are shut down and a {@link HoodieException} is thrown so the 
caller can exit with a non-zero status.
+   * Otherwise, every table is synced independently and a single failure does 
not affect the others.
+   */
+  private void syncContinuously() {
+    if (tableExecutionContexts.isEmpty()) {
+      return;
+    }
+    // Streamer instances are registered from worker threads, so a thread-safe 
list is required.
+    final List<HoodieStreamer> streamerInstances = new 
CopyOnWriteArrayList<>();
+    // Set once fail fast trips, so tasks that register their streamer 
afterwards stop before starting the sync.
+    final AtomicBoolean shutdownRequested = new AtomicBoolean(false);
+    final ExecutorService executor = 
Executors.newFixedThreadPool(tableExecutionContexts.size(),
+        new CustomizedThreadFactory("multi-table-streamer", true));
+    boolean terminated = false;
+    try {
+      final List<CompletableFuture<Void>> tableFutures = 
tableExecutionContexts.stream()
+          .map(context -> CompletableFuture.runAsync(
+              () -> runTableSync(context, streamerInstances, 
shutdownRequested), executor))
+          .collect(Collectors.toList());
+
+      if (failFastOnContinuousMode) {
+        log.info("Fail fast enabled in continuous mode. The whole job fails on 
any single table failure");
+        awaitFailFast(tableFutures, streamerInstances, shutdownRequested);
+      } else {
+        CompletableFuture.allOf(tableFutures.toArray(new 
CompletableFuture[0])).join();
+      }
+    } finally {
+      // Wait for every worker thread to finish (including its finally 
cleanup) before returning, so sync() does not
+      // return while a table is still writing and main() then stops the 
shared Spark context under it.
+      terminated = shutdownExecutor(executor);
+    }
+    // If the workers never terminated, ingestion may still be running. Fail 
loudly instead of returning as if the
+    // cleanup succeeded, so the caller does not silently proceed to Spark 
teardown with live writers.
+    if (!terminated) {
+      throw new HoodieException("Timed out shutting down table ingestion 
workers in continuous mode");
+    }
+  }
+
+  /**
+   * Syncs one table on the calling worker thread. Rethrows only under fail 
fast; otherwise the failure is recorded
+   * in {@link #failedTables} and the sibling tables carry on.
+   */
+  private void runTableSync(TableExecutionContext context, 
List<HoodieStreamer> streamerInstances, AtomicBoolean shutdownRequested) {
+    HoodieStreamer streamer = null;
+    try {
+      streamer = new HoodieStreamer(context.getConfig(), jssc, 
Option.ofNullable(context.getProperties()));
+      streamerInstances.add(streamer);
+      // Register before checking the flag so a concurrent shutdownStreamers() 
always sees this streamer.
+      if (shutdownRequested.get()) {
+        return;
+      }
+      streamer.sync();
+      // A streamer registered just before fail fast tripped can reach here 
without ever ingesting.
+      // shutdown() call will be a no-op because its ingestion service hadn't 
started yet.
+      // Don't count that as a success.
+      if (!shutdownRequested.get()) {
+        successTables.add(Helpers.getTableWithDatabase(context));
+      }
+    } catch (Exception e) {
+      String table = Helpers.getTableWithDatabase(context);
+      log.error("error while running MultiTableDeltaStreamer for table: {}", 
table, e);
+      failedTables.add(table);
+      if (failFastOnContinuousMode) {
+        // Name the table so the thrown exception identifies the culprit, not 
the siblings torn down after it.
+        throw new HoodieException("Table sync failed in continuous mode for 
table: " + table, e);

Review Comment:
   The new per-table message is what makes the failing table identifiable, but 
assertThrows only checks the type so no test looks at it. Worth capturing the 
thrown exception in testFailFastOnContinuousThrowsWhenATableFails and asserting 
its cause names the table that actually failed.



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

Reply via email to