From 7d246178e3df720414005492c81463a481221790 Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Thu, 27 Aug 2026 02:10:50 +0800 Subject: [PATCH 01/10] [api][core] Parallelize format table overwrite cleanup A Format Table INSERT OVERWRITE deletes the old data files it replaces one at a time on the driver. On a table holding thousands of files that is thousands of synchronous round trips after the last task has finished, with nothing else running. Hand those deletes to a bounded runner. Only a catalog-managed partitioned Format Table uses it; filesystem-discovered, unpartitioned, truncate and ordinary Paimon table paths stay serial, and format-table.commit.cleanup-thread-num = 1 opts out. The runner is the bounded batch execution in ThreadPoolUtils, extended here for callers that change stored state: Let it take an iterator, so a caller that discovers its work by listing storage does not have to list all of it before the first task can start, and refill the window as a slot frees rather than a batch at a time. An overwrite that replaces the whole table then holds one partition rather than every file the table has. Add a variant whose close waits for a task that has already started instead of interrupting it. Interrupting a delete halfway leaves the caller unable to say whether it took effect, so a caller that changes stored state cannot let close cancel what it has already handed out. Give a worker its thread's classloader back, and clear the interrupt a cancelled task may leave behind, so that neither reaches whatever the shared pool runs next. Everything accepted is waited for before the commit fails, and failures keep their input order. --- docs/generated/core_configuration.html | 6 + .../java/org/apache/paimon/CoreOptions.java | 23 + .../apache/paimon/utils/ThreadPoolUtils.java | 152 +- .../paimon/utils/ThreadPoolUtilsTest.java | 229 ++- .../table/format/FormatBatchWriteBuilder.java | 7 +- .../table/format/FormatTableCommit.java | 197 ++- .../paimon/utils/ManifestReadThreadPool.java | 2 +- .../org/apache/paimon/CoreOptionsTest.java | 37 + .../FormatTableCommitStatisticsTest.java | 3 +- .../table/format/FormatTableCommitTest.java | 1270 ++++++++++++++++- 10 files changed, 1801 insertions(+), 125 deletions(-) diff --git a/docs/generated/core_configuration.html b/docs/generated/core_configuration.html index 3fb25ebce15f..e544bec90fb5 100644 --- a/docs/generated/core_configuration.html +++ b/docs/generated/core_configuration.html @@ -746,6 +746,12 @@ String Format table commit hive sync uri. + +
format-table.commit.cleanup-thread-num
+ 64 + Integer + The maximum number of concurrent deletions of old data files during overwrite commits for an internal Format Table with catalog-managed partitions. Supported values are 1 through 64. Other Format Tables use serial cleanup. This limit uses a separate thread pool and is independent of file-operation.thread-num, so the total file-operation concurrency in one process may be the sum of both limits. +
format-table.file.compression
(none) diff --git a/paimon-api/src/main/java/org/apache/paimon/CoreOptions.java b/paimon-api/src/main/java/org/apache/paimon/CoreOptions.java index a19d1818c247..363aec22b29e 100644 --- a/paimon-api/src/main/java/org/apache/paimon/CoreOptions.java +++ b/paimon-api/src/main/java/org/apache/paimon/CoreOptions.java @@ -2654,6 +2654,19 @@ public String toString() { .noDefaultValue() .withDescription("Format table commit hive sync uri."); + public static final ConfigOption FORMAT_TABLE_COMMIT_CLEANUP_THREAD_NUM = + key("format-table.commit.cleanup-thread-num") + .intType() + .defaultValue(64) + .withDescription( + "The maximum number of concurrent deletions of old data files during " + + "overwrite commits for an internal Format Table with " + + "catalog-managed partitions. Supported values are 1 through " + + "64. Other Format Tables use serial cleanup. This limit uses " + + "a separate thread pool and is independent of " + + "file-operation.thread-num, so the total file-operation " + + "concurrency in one process may be the sum of both limits."); + @Immutable public static final ConfigOption BLOB_FIELD = key("blob-field") @@ -3302,6 +3315,16 @@ public String formatTableCommitSyncPartitionHiveUri() { return options.get(FORMAT_TABLE_COMMIT_HIVE_SYNC_URI); } + public int formatTableCommitCleanupThreadNum() { + int threadNum = options.get(FORMAT_TABLE_COMMIT_CLEANUP_THREAD_NUM); + checkArgument( + threadNum >= 1 && threadNum <= 64, + "Option %s must be between 1 and 64, but was %s.", + FORMAT_TABLE_COMMIT_CLEANUP_THREAD_NUM.key(), + threadNum); + return threadNum; + } + public MemorySize fileReaderAsyncThreshold() { return options.get(FILE_READER_ASYNC_THRESHOLD); } diff --git a/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java b/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java index b5c28a19a08a..e91ba169e304 100644 --- a/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java +++ b/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java @@ -134,7 +134,7 @@ private void advanceIfNeeded() { } /** - * Parallel processes one bounded batch at a time and returns results in input order. + * Processes a bounded number of inputs in parallel and returns results in input order. * *

The caller must close the iterator to cancel unstarted tasks and wait for running tasks. */ @@ -143,10 +143,36 @@ public static CloseableBatchIterator sequentialBatchedExecuteCloseable Function> processor, List input, int queueSize) { + return newSequentialBatchIterator(executor, processor, input.iterator(), queueSize, true); + } + + /** + * As {@link #sequentialBatchedExecuteCloseable}, but closing waits for a task that has already + * started instead of interrupting it. + * + *

Use this when a task changes stored state. Interrupting a delete or a write halfway leaves + * the caller unable to say whether it took effect, so a caller that has to know the outcome of + * everything it handed out cannot let close cancel work that is already running. + */ + public static CloseableBatchIterator sequentialBatchedExecuteAwaitRunningTasksOnClose( + ExecutorService executor, + Function> processor, + Iterator input, + int queueSize) { + return newSequentialBatchIterator(executor, processor, input, queueSize, false); + } + + private static CloseableBatchIterator newSequentialBatchIterator( + ExecutorService executor, + Function> processor, + Iterator input, + int queueSize, + boolean cancelRunningOnClose) { if (queueSize <= 0) { throw new NegativeArraySizeException("queue size should not be negative"); } - return new SequentialBatchIterator<>(executor, processor, input, queueSize); + return new SequentialBatchIterator<>( + executor, processor, input, queueSize, cancelRunningOnClose); } public static void randomlyOnlyExecute( @@ -224,21 +250,28 @@ private static class SequentialBatchIterator implements CloseableBatchIter private final ExecutorService executor; private final Function> processor; - private final Queue> batches; + private final Iterator input; + private final int queueSize; + private final boolean cancelRunningOnClose; private final Queue> activeTasks = new ArrayDeque<>(); + private final Object submissionLock = new Object(); private Iterator activeResults = Collections.emptyList().iterator(); private T next; private boolean closed; + private boolean submissionStopped; private SequentialBatchIterator( ExecutorService executor, Function> processor, - List input, - int queueSize) { + Iterator input, + int queueSize, + boolean cancelRunningOnClose) { this.executor = executor; this.processor = processor; - this.batches = new ArrayDeque<>(Lists.partition(input, queueSize)); + this.input = input; + this.queueSize = queueSize; + this.cancelRunningOnClose = cancelRunningOnClose; } @Override @@ -263,32 +296,46 @@ private void advanceIfNeeded() { while (next == null) { if (activeResults.hasNext()) { next = activeResults.next(); - } else if (!activeTasks.isEmpty()) { - BatchTask task = activeTasks.peek(); - try { - List results = task.result(); + continue; + } + fillWindow(); + if (activeTasks.isEmpty()) { + return; + } + BatchTask task = activeTasks.peek(); + try { + List results = task.result(); + activeTasks.poll(); + activeResults = results.iterator(); + } catch (RuntimeException | Error failure) { + if (task.failureReported()) { activeTasks.poll(); - activeResults = results.iterator(); - } catch (RuntimeException | Error failure) { - if (task.failureReported()) { - activeTasks.poll(); - } - throw failure; } - } else if (batches.isEmpty()) { - return; - } else { - submitBatch(batches.poll()); + throw failure; } } } - private void submitBatch(List batch) { + /** Does not consume more input than the active-task window can hold. */ + private void fillWindow() { ClassLoader classLoader = Thread.currentThread().getContextClassLoader(); - for (U input : batch) { - BatchTask task = new BatchTask<>(processor, input, classLoader); - executor.execute(task); - activeTasks.add(task); + while (activeTasks.size() < queueSize) { + synchronized (submissionLock) { + if (submissionStopped || !input.hasNext()) { + return; + } + BatchTask task = + new BatchTask<>( + processor, input.next(), classLoader, this::stopSubmission); + executor.execute(task); + activeTasks.add(task); + } + } + } + + private void stopSubmission() { + synchronized (submissionLock) { + submissionStopped = true; } } @@ -298,17 +345,25 @@ public synchronized void close() { return; } closed = true; - batches.clear(); Throwable failure = null; boolean interrupted = Thread.interrupted(); for (BatchTask task : activeTasks) { try { - task.cancel(); + task.cancelIfUnstarted(); } catch (Throwable cleanupFailure) { failure = firstOrSuppressed(cleanupFailure, failure); } } + if (cancelRunningOnClose) { + for (BatchTask task : activeTasks) { + try { + task.interruptIfRunning(); + } catch (Throwable cleanupFailure) { + failure = firstOrSuppressed(cleanupFailure, failure); + } + } + } for (BatchTask task : activeTasks) { while (true) { try { @@ -346,6 +401,7 @@ private static class BatchTask implements Runnable { private final Function> processor; private final U input; private final ClassLoader classLoader; + private final Runnable stopSubmission; private final CountDownLatch completion = new CountDownLatch(1); private int state = CREATED; @@ -354,10 +410,15 @@ private static class BatchTask implements Runnable { private Throwable failure; private volatile boolean failureReported; - private BatchTask(Function> processor, U input, ClassLoader classLoader) { + private BatchTask( + Function> processor, + U input, + ClassLoader classLoader, + Runnable stopSubmission) { this.processor = processor; this.input = input; this.classLoader = classLoader; + this.stopSubmission = stopSubmission; } @Override @@ -372,12 +433,23 @@ public void run() { runner = Thread.currentThread(); } + Thread currentThread = Thread.currentThread(); + boolean interruptedOnEntry = currentThread.isInterrupted(); + ClassLoader originalClassLoader = currentThread.getContextClassLoader(); try { - Thread.currentThread().setContextClassLoader(classLoader); + currentThread.setContextClassLoader(classLoader); result = processor.apply(input); } catch (RuntimeException | Error taskFailure) { failure = taskFailure; + stopSubmission.run(); } finally { + currentThread.setContextClassLoader(originalClassLoader); + // Reset the flag to its entry state so a cancelled task cannot leak an interrupt + // to a reused worker and a direct executor cannot clear its caller's interrupt. + Thread.interrupted(); + if (interruptedOnEntry) { + currentThread.interrupt(); + } synchronized (this) { runner = null; state = FINISHED; @@ -386,21 +458,27 @@ public void run() { } } - private synchronized void cancel() { + private synchronized void interruptIfRunning() { + if (state == RUNNING) { + runner.interrupt(); + } + } + + private synchronized void cancelIfUnstarted() { if (state == CREATED) { state = CANCELLED; completion.countDown(); - } else if (state == RUNNING) { - runner.interrupt(); } } private List result() { - try { - completion.await(); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - throw new RuntimeException(e); + if (completion.getCount() != 0) { + try { + completion.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException(e); + } } if (failure != null) { failureReported = true; diff --git a/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java b/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java index 4bb556343f55..899c7842fffc 100644 --- a/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java +++ b/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java @@ -20,11 +20,15 @@ import org.apache.paimon.utils.ThreadPoolUtils.CloseableBatchIterator; +import org.apache.paimon.shade.guava30.com.google.common.collect.Iterators; +import org.apache.paimon.shade.guava30.com.google.common.util.concurrent.MoreExecutors; + import org.junit.jupiter.api.Test; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.Iterator; import java.util.List; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; @@ -45,10 +49,11 @@ public class ThreadPoolUtilsTest { @Test public void testCloseableBatchReturnsInOrderAndBoundsSubmission() throws Exception { - CountingThreadPoolExecutor workers = new CountingThreadPoolExecutor(2); + ThreadPoolExecutor workers = (ThreadPoolExecutor) Executors.newFixedThreadPool(2); ExecutorService consumer = Executors.newSingleThreadExecutor(); CountDownLatch firstStarted = new CountDownLatch(1); CountDownLatch secondFinished = new CountDownLatch(1); + CountDownLatch thirdStarted = new CountDownLatch(1); CountDownLatch releaseFirst = new CountDownLatch(1); CloseableBatchIterator iterator = ThreadPoolUtils.sequentialBatchedExecuteCloseable( @@ -59,6 +64,8 @@ public void testCloseableBatchReturnsInOrderAndBoundsSubmission() throws Excepti await(releaseFirst); } else if (input == 1) { secondFinished.countDown(); + } else if (input == 2) { + thirdStarted.countDown(); } return Collections.singletonList(input); }, @@ -75,18 +82,19 @@ public void testCloseableBatchReturnsInOrderAndBoundsSubmission() throws Excepti assertThat(firstStarted.await(3, TimeUnit.SECONDS)).isTrue(); assertThat(secondFinished.await(3, TimeUnit.SECONDS)).isTrue(); - assertThat(workers.getSubmittedTaskCount()).isEqualTo(2); + workers.submit(() -> {}).get(3, TimeUnit.SECONDS); + assertThat(thirdStarted.getCount()).isOne(); assertThat(firstResult.isDone()).isFalse(); releaseFirst.countDown(); List results = new ArrayList<>(); results.add(firstResult.get(3, TimeUnit.SECONDS)); + // Consuming input 0 frees one slot. The next lookup refills it before input 1 is + // consumed, instead of waiting for the whole window to drain. assertThat(iterator.hasNext()).isTrue(); + assertThat(thirdStarted.await(3, TimeUnit.SECONDS)).isTrue(); results.add(iterator.next()); - assertThat(workers.getSubmittedTaskCount()).isEqualTo(2); - assertThat(iterator.hasNext()).isTrue(); - assertThat(workers.getSubmittedTaskCount()).isEqualTo(4); results.add(iterator.next()); assertThat(iterator.hasNext()).isTrue(); results.add(iterator.next()); @@ -102,9 +110,189 @@ public void testCloseableBatchReturnsInOrderAndBoundsSubmission() throws Excepti } } + @Test + public void testLazyInputIsConsumedOnlyAsSlotsFree() throws Exception { + ExecutorService workers = Executors.newFixedThreadPool(2); + CountDownLatch releaseFirst = new CountDownLatch(1); + AtomicInteger inputsRead = new AtomicInteger(); + Iterator input = + new Iterator() { + private int next; + + @Override + public boolean hasNext() { + return next < 100; + } + + @Override + public Integer next() { + inputsRead.incrementAndGet(); + return next++; + } + }; + try (CloseableBatchIterator iterator = + ThreadPoolUtils.sequentialBatchedExecuteAwaitRunningTasksOnClose( + workers, + value -> { + if (value == 0) { + await(releaseFirst); + } + return Collections.singletonList(value); + }, + input, + 4)) { + releaseFirst.countDown(); + assertThat(iterator.next()).isEqualTo(0); + assertThat(inputsRead).hasValue(4); + } finally { + workers.shutdownNow(); + } + } + + @Test + public void testWorkerFailureStopsNewSubmissions() throws Exception { + ExecutorService workers = Executors.newFixedThreadPool(2); + ExecutorService consumer = Executors.newSingleThreadExecutor(); + CountDownLatch secondFailed = new CountDownLatch(1); + CountDownLatch releaseFirst = new CountDownLatch(1); + AtomicInteger inputsRead = new AtomicInteger(); + RuntimeException workerFailure = new RuntimeException("worker failure"); + Iterator input = + Iterators.transform( + Arrays.asList(0, 1, 2).iterator(), + value -> { + inputsRead.incrementAndGet(); + return value; + }); + CloseableBatchIterator iterator = + ThreadPoolUtils.sequentialBatchedExecuteAwaitRunningTasksOnClose( + workers, + value -> { + if (value == 0) { + await(releaseFirst); + } else if (value == 1) { + secondFailed.countDown(); + throw workerFailure; + } + return Collections.singletonList(value); + }, + input, + 2); + + try { + Future result = + consumer.submit( + () -> + catchThrowable( + () -> { + assertThat(iterator.next()).isZero(); + iterator.hasNext(); + })); + + assertThat(secondFailed.await(3, TimeUnit.SECONDS)).isTrue(); + // With input 0 still gated, this can only run after the failed task has + // completely left BatchTask.run and published its failure state. + workers.submit(() -> {}).get(3, TimeUnit.SECONDS); + assertThat(inputsRead).hasValue(2); + + releaseFirst.countDown(); + assertThat(result.get(3, TimeUnit.SECONDS)).isSameAs(workerFailure); + assertThat(inputsRead).hasValue(2); + } finally { + releaseFirst.countDown(); + iterator.close(); + consumer.shutdownNow(); + workers.shutdownNow(); + assertThat(consumer.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); + assertThat(workers.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); + } + } + + @Test + public void testWorkerUsesCallerClassLoaderAndRestoresPoolClassLoader() throws Exception { + ExecutorService workers = Executors.newFixedThreadPool(1); + ClassLoader callerClassLoader = new ClassLoader(getClass().getClassLoader()) {}; + AtomicReference poolClassLoader = new AtomicReference<>(); + workers.submit(() -> poolClassLoader.set(Thread.currentThread().getContextClassLoader())) + .get(10, TimeUnit.SECONDS); + + ClassLoader original = Thread.currentThread().getContextClassLoader(); + List seen = new ArrayList<>(); + try { + Thread.currentThread().setContextClassLoader(callerClassLoader); + try (CloseableBatchIterator iterator = + ThreadPoolUtils.sequentialBatchedExecuteCloseable( + workers, + value -> { + seen.add(Thread.currentThread().getContextClassLoader()); + return Collections.singletonList(value); + }, + Arrays.asList(0, 1), + 1)) { + while (iterator.hasNext()) { + iterator.next(); + } + } + } finally { + Thread.currentThread().setContextClassLoader(original); + } + + assertThat(seen).containsExactly(callerClassLoader, callerClassLoader); + // The pool is shared, so a worker that keeps a caller's loader would hand it to whatever + // runs on that thread next. + AtomicReference restoredPoolClassLoader = new AtomicReference<>(); + workers.submit( + () -> + restoredPoolClassLoader.set( + Thread.currentThread().getContextClassLoader())) + .get(10, TimeUnit.SECONDS); + assertThat(restoredPoolClassLoader.get()).isSameAs(poolClassLoader.get()); + workers.shutdownNow(); + } + + @Test + public void testDirectExecutorPreservesCallerInterrupt() { + ExecutorService workers = MoreExecutors.newDirectExecutorService(); + try { + Thread.currentThread().interrupt(); + try (CloseableBatchIterator iterator = + ThreadPoolUtils.sequentialBatchedExecuteCloseable( + workers, Collections::singletonList, Collections.singletonList(1), 1)) { + assertThat(iterator.next()).isOne(); + assertThat(Thread.currentThread().isInterrupted()).isTrue(); + } + } finally { + Thread.interrupted(); + workers.shutdownNow(); + } + } + @Test public void testCloseCancelsQueuedTasksAndWaitsUninterruptibly() throws Exception { - ThreadPoolExecutor workers = (ThreadPoolExecutor) Executors.newFixedThreadPool(1); + LinkedBlockingQueue taskQueue = new LinkedBlockingQueue<>(); + AtomicBoolean runQueuedTaskOnInterrupt = new AtomicBoolean(); + // If close interrupts the worker before cancelling queued tasks, interrupt() runs the + // queued task and exposes the ordering bug. + ThreadPoolExecutor workers = + new ThreadPoolExecutor( + 1, + 1, + 0L, + TimeUnit.MILLISECONDS, + taskQueue, + runnable -> + new Thread(runnable) { + @Override + public void interrupt() { + super.interrupt(); + if (runQueuedTaskOnInterrupt.compareAndSet(true, false)) { + Runnable queuedTask = taskQueue.poll(); + if (queuedTask != null) { + queuedTask.run(); + } + } + } + }); ExecutorService closer = Executors.newSingleThreadExecutor(); CountDownLatch secondStarted = new CountDownLatch(1); CountDownLatch workerInterrupted = new CountDownLatch(1); @@ -134,7 +322,9 @@ public void testCloseCancelsQueuedTasksAndWaitsUninterruptibly() throws Exceptio assertThat(iterator.hasNext()).isTrue(); assertThat(iterator.next()).isZero(); assertThat(secondStarted.await(3, TimeUnit.SECONDS)).isTrue(); - assertThat(workers.getTaskCount()).isEqualTo(3); + assertThat(workers.getQueue()).hasSize(1); + assertThat(thirdExecuted).isFalse(); + runQueuedTaskOnInterrupt.set(true); Future closeResult = closer.submit( @@ -146,6 +336,7 @@ public void testCloseCancelsQueuedTasksAndWaitsUninterruptibly() throws Exceptio }); assertThat(closeStarted.await(3, TimeUnit.SECONDS)).isTrue(); assertThat(workerInterrupted.await(3, TimeUnit.SECONDS)).isTrue(); + assertThat(runQueuedTaskOnInterrupt).isFalse(); assertThat(closeResult.isDone()).isFalse(); closeThread.get().interrupt(); @@ -236,28 +427,4 @@ private static void awaitIgnoringInterrupts(CountDownLatch latch, CountDownLatch } } } - - private static class CountingThreadPoolExecutor extends ThreadPoolExecutor { - - private final AtomicInteger submittedTaskCount = new AtomicInteger(); - - private CountingThreadPoolExecutor(int threadCount) { - super(threadCount, threadCount, 0L, TimeUnit.MILLISECONDS, new LinkedBlockingQueue<>()); - } - - @Override - public void execute(Runnable command) { - submittedTaskCount.incrementAndGet(); - try { - super.execute(command); - } catch (RuntimeException | Error failure) { - submittedTaskCount.decrementAndGet(); - throw failure; - } - } - - private int getSubmittedTaskCount() { - return submittedTaskCount.get(); - } - } } diff --git a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatBatchWriteBuilder.java b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatBatchWriteBuilder.java index 73c11d774166..4c8beca9c175 100644 --- a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatBatchWriteBuilder.java +++ b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatBatchWriteBuilder.java @@ -78,6 +78,10 @@ public BatchTableCommit newCommit() { CoreOptions options = new CoreOptions(table.options()); boolean formatTablePartitionOnlyValueInPath = options.formatTablePartitionOnlyValueInPath(); String syncHiveUri = options.formatTableCommitSyncPartitionHiveUri(); + int cleanupThreadNum = + table.partitionManager() != null && !table.partitionKeys().isEmpty() + ? options.formatTableCommitCleanupThreadNum() + : 1; return new FormatTableCommit( table.location(), table.partitionKeys(), @@ -90,7 +94,8 @@ public BatchTableCommit newCommit() { syncHiveUri, table.catalogContext(), table.partitionManager(), - options.dynamicPartitionOverwrite()); + options.dynamicPartitionOverwrite(), + cleanupThreadNum); } @Override diff --git a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java index be2105930a57..e54f67b557f1 100644 --- a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java +++ b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java @@ -38,6 +38,9 @@ import org.apache.paimon.table.sink.TableCommit; import org.apache.paimon.utils.Pair; import org.apache.paimon.utils.PartitionPathUtils; +import org.apache.paimon.utils.ThreadPoolUtils; + +import org.apache.paimon.shade.guava30.com.google.common.collect.Iterators; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -46,24 +49,35 @@ import java.io.FileNotFoundException; import java.io.IOException; +import java.io.UncheckedIOException; import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Collections; import java.util.HashSet; +import java.util.Iterator; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.ExecutorService; import java.util.stream.Collectors; import static org.apache.paimon.table.format.FormatBatchWriteBuilder.validateStaticPartition; +import static org.apache.paimon.utils.ThreadPoolUtils.CloseableBatchIterator; +import static org.apache.paimon.utils.ThreadPoolUtils.sequentialBatchedExecuteAwaitRunningTasksOnClose; /** Commit for Format Table. */ public class FormatTableCommit implements BatchTableCommit { private static final Logger LOG = LoggerFactory.getLogger(FormatTableCommit.class); + private static final int MAX_CLEANUP_THREAD_NUM = 64; + + private static final ExecutorService CLEANUP_EXECUTOR = + ThreadPoolUtils.createCachedThreadPool( + MAX_CLEANUP_THREAD_NUM, "FORMAT-TABLE-COMMIT-CLEANUP-THREAD-POOL"); + private String location; private final boolean formatTablePartitionOnlyValueInPath; private final String defaultPartName; @@ -75,6 +89,7 @@ public class FormatTableCommit implements BatchTableCommit { private Identifier tableIdentifier; @Nullable private final FormatTablePartitionManager partitionManager; private final boolean dynamicPartitionOverwrite; + private final int cleanupThreadNum; public FormatTableCommit( String location, @@ -88,7 +103,14 @@ public FormatTableCommit( @Nullable String syncHiveUri, CatalogContext catalogContext, @Nullable FormatTablePartitionManager partitionManager, - boolean dynamicPartitionOverwrite) { + boolean dynamicPartitionOverwrite, + int cleanupThreadNum) { + if (cleanupThreadNum < 1 || cleanupThreadNum > MAX_CLEANUP_THREAD_NUM) { + throw new IllegalArgumentException( + String.format( + "Format Table cleanup thread number must be between 1 and %s, but was %s.", + MAX_CLEANUP_THREAD_NUM, cleanupThreadNum)); + } this.location = location; this.fileIO = fileIO; this.formatTablePartitionOnlyValueInPath = formatTablePartitionOnlyValueInPath; @@ -100,6 +122,7 @@ public FormatTableCommit( this.tableIdentifier = tableIdentifier; this.partitionManager = partitionManager; this.dynamicPartitionOverwrite = dynamicPartitionOverwrite; + this.cleanupThreadNum = cleanupThreadNum; if (syncHiveUri != null) { try { Options options = new Options(); @@ -151,32 +174,31 @@ public void commit(List commitMessages) { // A static partition may name only the leading keys, in which case the path // is a prefix and the partition directories of the remaining keys sit below. clearedPartitionPaths.addAll( - deletePreviousDataFile( - partitionPath, partitionKeys.size() - staticPartitions.size())); + deletePreviousDataFiles( + Collections.singletonList(partitionPath), + partitionKeys.size() - staticPartitions.size(), + cleanupThreadNum)); } if (!fileIO.exists(partitionPath)) { fileIO.mkdirs(partitionPath); } } else if (overwrite) { if (replacesOnlyWrittenPartitions()) { - Set partitionPaths = new HashSet<>(); + Set partitionPaths = new LinkedHashSet<>(); for (TwoPhaseCommitMessage message : messages) { partitionPaths.add(message.getCommitter().targetPath().getParent()); } - for (Path p : partitionPaths) { - // The parent of a written file is a complete partition directory - the - // table directory itself when the table is unpartitioned - so there is no - // partition level below it to descend, and it is a partition this commit - // writes anyway. - deletePreviousDataFile(p, 0); - } + // The parent of a written file is a complete partition directory - the table + // directory itself when the table is unpartitioned - so there is no partition + // level below it to descend. Collect every selected directory before deleting + // so many small partitions can share the same cleanup concurrency window. + deletePreviousDataFiles(new ArrayList<>(partitionPaths), 0, cleanupThreadNum); } else { // Overwriting without naming a partition replaces the table, so what has to go // is everything the table holds rather than the files this commit happens to // write: a statement whose query returns nothing still empties the table. - for (Path dataDirectory : tableDataDirectories()) { - clearedPartitionPaths.addAll(deletePreviousDataFile(dataDirectory, 0)); - } + clearedPartitionPaths.addAll( + deletePreviousDataFiles(tableDataDirectories(), 0, cleanupThreadNum)); } } @@ -244,9 +266,26 @@ public void commit(List commitMessages) { } } - } catch (Exception e) { - this.abort(commitMessages); - throw new RuntimeException(e); + } catch (Throwable failure) { + // Cleanup restores the caller's interrupt before failing. Clear it only while aborting + // staging output, then restore it; an abort failure is secondary to the commit failure + // that made abort necessary. + boolean interrupted = Thread.interrupted(); + try { + this.abort(commitMessages); + } catch (Throwable abortFailure) { + if (failure != abortFailure) { + failure.addSuppressed(abortFailure); + } + } finally { + if (interrupted) { + Thread.currentThread().interrupt(); + } + } + if (failure instanceof Error) { + throw (Error) failure; + } + throw new RuntimeException(failure); } } @@ -506,42 +545,110 @@ private List, Path>> partitionsInTheFileSyste */ private Set deletePreviousDataFile(Path partitionPath, int partitionLevels) throws IOException { + return deletePreviousDataFiles( + Collections.singletonList(partitionPath), partitionLevels, 1); + } + + private Set deletePreviousDataFiles( + List partitionPaths, int partitionLevels, int threadNum) throws IOException { + Iterator dataFiles = previousDataFiles(partitionPaths, partitionLevels); Set clearedPartitionPaths = new HashSet<>(); - if (fileIO.exists(partitionPath)) { - // Committed data files only: what sits under a staging directory is another writer's - // uncommitted output, whatever its name looks like. - for (FileStatus file : - FormatTableScan.listDataFiles( - fileIO, - partitionPath, - partitionLevels, - formatTablePartitionOnlyValueInPath, - defaultPartName)) { - boolean deleted; - try { - deleted = fileIO.delete(file.getPath(), false); - } catch (FileNotFoundException ignore) { - continue; - } catch (IOException e) { - throw new RuntimeException(e); + try { + if (threadNum == 1) { + while (dataFiles.hasNext()) { + FileStatus file = dataFiles.next(); + if (deleteDataFile(file)) { + clearedPartitionPaths.add(file.getPath().getParent()); + } } - if (deleted) { - // Only what this commit removed: a file another writer deleted first would - // have every concurrent writer report the whole subtree. - clearedPartitionPaths.add(file.getPath().getParent()); - } else if (fileIO.exists(file.getPath())) { - // A refusal is not that race: the file is still readable, and going on would - // report the partition as holding nothing while its rows are still there. - throw new IOException( - String.format( - "Failed to delete data file %s of table %s.", - file.getPath(), tableIdentifier.getFullName())); + return clearedPartitionPaths; + } + // Listing lazily keeps the memory of an overwrite that replaces the table + // proportional to one partition rather than to everything the table holds. Closing + // the iterator is what stops new deletes and waits for the ones already handed out, + // so a failure cannot leave a worker still deleting after this method returns. + try (CloseableBatchIterator cleared = + sequentialBatchedExecuteAwaitRunningTasksOnClose( + CLEANUP_EXECUTOR, this::deleteAndReportCleared, dataFiles, threadNum)) { + while (cleared.hasNext()) { + clearedPartitionPaths.add(cleared.next()); } } + } catch (UncheckedIOException e) { + throw (IOException) unwrapUncheckedIOException(e); } return clearedPartitionPaths; } + /** Unwraps worker I/O failures while preserving recursively suppressed failures. */ + private static Throwable unwrapUncheckedIOException(Throwable failure) { + if (!(failure instanceof UncheckedIOException)) { + return failure; + } + Throwable unwrapped = failure.getCause(); + for (Throwable suppressed : failure.getSuppressed()) { + unwrapped.addSuppressed(unwrapUncheckedIOException(suppressed)); + } + return unwrapped; + } + + /** Deletes one file and reports the partition it emptied, for a worker that cannot throw. */ + private List deleteAndReportCleared(FileStatus file) { + try { + return deleteDataFile(file) + ? Collections.singletonList(file.getPath().getParent()) + : Collections.emptyList(); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + + /** Deletes one listed data file and reports whether this commit removed it. */ + private boolean deleteDataFile(FileStatus file) throws IOException { + try { + if (fileIO.delete(file.getPath(), false)) { + return true; + } + } catch (FileNotFoundException ignore) { + return false; + } + if (fileIO.exists(file.getPath())) { + // A refusal is not a concurrent-delete race: the file is still readable, and going on + // would report the partition as holding nothing while its rows are still there. + throw new IOException( + String.format( + "Failed to delete data file %s of table %s.", + file.getPath(), tableIdentifier.getFullName())); + } + return false; + } + + /** The committed data files below the given paths, listed one partition at a time. */ + private Iterator previousDataFiles(List partitionPaths, int partitionLevels) { + return Iterators.concat( + Iterators.transform( + partitionPaths.iterator(), + partitionPath -> { + try { + if (!fileIO.exists(partitionPath)) { + return Collections.emptyList().iterator(); + } + // Committed data files only: what sits under a staging directory is + // another writer's uncommitted output, whatever its name looks + // like. + return FormatTableScan.listDataFiles( + fileIO, + partitionPath, + partitionLevels, + formatTablePartitionOnlyValueInPath, + defaultPartName) + .iterator(); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + })); + } + @Override public void truncateTable() { // Data files only. The partition directories stay, and so do their catalog registrations: diff --git a/paimon-core/src/main/java/org/apache/paimon/utils/ManifestReadThreadPool.java b/paimon-core/src/main/java/org/apache/paimon/utils/ManifestReadThreadPool.java index 0ef818762d40..1e106cb3d4e1 100644 --- a/paimon-core/src/main/java/org/apache/paimon/utils/ManifestReadThreadPool.java +++ b/paimon-core/src/main/java/org/apache/paimon/utils/ManifestReadThreadPool.java @@ -61,7 +61,7 @@ public static Iterable sequentialBatchedExecute( executor, processor, input, effectiveThreadNum(threadNum, executor)); } - /** This method parallel processes one bounded batch and waits for it when closed. */ + /** Processes a bounded number of inputs in parallel and waits for submitted tasks on close. */ public static ThreadPoolUtils.CloseableBatchIterator sequentialBatchedExecuteCloseable( Function> processor, List input, @Nullable Integer threadNum) { diff --git a/paimon-core/src/test/java/org/apache/paimon/CoreOptionsTest.java b/paimon-core/src/test/java/org/apache/paimon/CoreOptionsTest.java index ac71ff2552aa..f3e81179d733 100644 --- a/paimon-core/src/test/java/org/apache/paimon/CoreOptionsTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/CoreOptionsTest.java @@ -288,4 +288,41 @@ public void testLocalKvDbBlockSize() { .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("local-kv-db.block-size"); } + + @Test + public void testFormatTableCommitCleanupThreadNumDefaultsTo64AndAcceptsBounds() { + Options conf = new Options(); + assertThat(new CoreOptions(conf).formatTableCommitCleanupThreadNum()).isEqualTo(64); + + conf.set(CoreOptions.FORMAT_TABLE_COMMIT_CLEANUP_THREAD_NUM, 1); + assertThat(new CoreOptions(conf).formatTableCommitCleanupThreadNum()).isEqualTo(1); + + conf.set(CoreOptions.FORMAT_TABLE_COMMIT_CLEANUP_THREAD_NUM, 64); + assertThat(new CoreOptions(conf).formatTableCommitCleanupThreadNum()).isEqualTo(64); + } + + @Test + public void testFormatTableCommitCleanupThreadNumRejectsValuesOutsideSupportedRange() { + Options conf = new Options(); + conf.set(CoreOptions.FORMAT_TABLE_COMMIT_CLEANUP_THREAD_NUM, 0); + assertThatThrownBy(() -> new CoreOptions(conf).formatTableCommitCleanupThreadNum()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("format-table.commit.cleanup-thread-num") + .hasMessageContaining("1") + .hasMessageContaining("64"); + + conf.set(CoreOptions.FORMAT_TABLE_COMMIT_CLEANUP_THREAD_NUM, -1); + assertThatThrownBy(() -> new CoreOptions(conf).formatTableCommitCleanupThreadNum()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("format-table.commit.cleanup-thread-num") + .hasMessageContaining("1") + .hasMessageContaining("64"); + + conf.set(CoreOptions.FORMAT_TABLE_COMMIT_CLEANUP_THREAD_NUM, 65); + assertThatThrownBy(() -> new CoreOptions(conf).formatTableCommitCleanupThreadNum()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("format-table.commit.cleanup-thread-num") + .hasMessageContaining("1") + .hasMessageContaining("64"); + } } diff --git a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitStatisticsTest.java b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitStatisticsTest.java index f5ac14607065..1a52eaab887d 100644 --- a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitStatisticsTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitStatisticsTest.java @@ -674,7 +674,8 @@ private FormatTableCommit commit( null, null, partitionManager, - dynamicPartitionOverwrite); + dynamicPartitionOverwrite, + /* cleanupThreadNum */ 1); } /** An overwrite that names no partition: INSERT OVERWRITE without a PARTITION clause. */ diff --git a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java index 75b141d947f2..77ac0ff03d3a 100644 --- a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java @@ -18,27 +18,51 @@ package org.apache.paimon.table.format; +import org.apache.paimon.CoreOptions; import org.apache.paimon.catalog.Identifier; +import org.apache.paimon.fs.FileIO; +import org.apache.paimon.fs.FileStatus; import org.apache.paimon.fs.Path; import org.apache.paimon.fs.RenamingTwoPhaseOutputStream; import org.apache.paimon.fs.TwoPhaseOutputStream; import org.apache.paimon.fs.local.LocalFileIO; import org.apache.paimon.partition.Partition; +import org.apache.paimon.partition.PartitionStatistics; +import org.apache.paimon.table.FormatTable; +import org.apache.paimon.table.sink.BatchWriteBuilder; import org.apache.paimon.table.sink.CommitMessage; +import org.apache.paimon.types.DataTypes; +import org.apache.paimon.types.RowType; import org.apache.paimon.utils.PartitionPathUtils; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import org.mockito.ArgumentCaptor; +import java.io.FileNotFoundException; import java.io.IOException; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.Comparator; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import static org.apache.paimon.CoreOptions.PARTITION_DEFAULT_NAME; +import static org.apache.paimon.shade.guava30.com.google.common.base.Throwables.getCausalChain; +import static org.apache.paimon.shade.guava30.com.google.common.base.Throwables.getRootCause; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assertions.entry; @@ -46,6 +70,7 @@ import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyList; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -86,7 +111,8 @@ void testPartitionRegistrationFailureDiscardsTheFilesItWrote() throws Exception null, null, partitionManager, - /* dynamicPartitionOverwrite */ true); + /* dynamicPartitionOverwrite */ true, + /* cleanupThreadNum */ 1); CommitMessage message = new TwoPhaseCommitMessage(committer); assertThatThrownBy(() -> commit.commit(Collections.singletonList(message))) @@ -119,7 +145,8 @@ void testFileCommitFailureStillDiscardsUncommittedFiles() throws Exception { null, null, partitionManager, - /* dynamicPartitionOverwrite */ true); + /* dynamicPartitionOverwrite */ true, + /* cleanupThreadNum */ 1); CommitMessage message = new TwoPhaseCommitMessage(committer); assertThatThrownBy(() -> commit.commit(Collections.singletonList(message))) @@ -215,7 +242,8 @@ void testOverwriteKeepsFilesOfConcurrentWritersStagingTrees() throws Exception { null, null, null, - /* dynamicPartitionOverwrite */ true); + /* dynamicPartitionOverwrite */ true, + /* cleanupThreadNum */ 1); commit.commit(Collections.singletonList(new TwoPhaseCommitMessage(committer))); @@ -264,7 +292,8 @@ void testOverwritingAPrefixKeepsStagingTreesSittingAtAPartitionLevel() throws Ex null, null, null, - /* dynamicPartitionOverwrite */ true); + /* dynamicPartitionOverwrite */ true, + /* cleanupThreadNum */ 1); commit.commit(Collections.emptyList()); @@ -308,7 +337,8 @@ void testOverwritingAPrefixClearsTheDefaultPartitionDirectory() throws Exception null, null, null, - /* dynamicPartitionOverwrite */ true); + /* dynamicPartitionOverwrite */ true, + /* cleanupThreadNum */ 1); commit.commit(Collections.emptyList()); @@ -356,7 +386,8 @@ void testValueOnlyStaticPartitionCannotEscapeTableLocation() throws Exception { null, null, null, - /* dynamicPartitionOverwrite */ true); + /* dynamicPartitionOverwrite */ true, + /* cleanupThreadNum */ 1); assertThatThrownBy(() -> commit.commit(Collections.emptyList())) .isInstanceOf(RuntimeException.class) @@ -632,6 +663,667 @@ void testOverwritingTheWholeTableLeavesADirectoryThatIsNoPartitionOfIt() throws assertThat(fileIO.exists(new Path(tablePath, "loose.csv"))).isTrue(); } + @Test + void testCatalogManagedBuilderUses64WayCleanupByDefault() throws Exception { + ParallelDeleteFileIO fileIO = new ParallelDeleteFileIO(64, true); + Path tablePath = new Path(tempDir.toUri()); + Path partitionPath = new Path(tablePath, "part=p"); + writeOldFiles(fileIO, partitionPath, 65); + FormatTablePartitionManager partitionManager = mock(FormatTablePartitionManager.class); + TwoPhaseOutputStream.Committer committer = mock(TwoPhaseOutputStream.Committer.class); + when(committer.targetPath()).thenReturn(new Path(partitionPath, "data-new.csv")); + doAnswer( + invocation -> { + assertThat(fileIO.activeDeletes()).isZero(); + return null; + }) + .when(committer) + .commit(fileIO); + FormatTableCommit commit = + builderOverwriteCommit( + tablePath, + fileIO, + partitionManager, + Collections.emptyMap(), + Collections.singletonMap("part", "p")); + + ExecutorService executor = Executors.newSingleThreadExecutor(); + try { + Future result = + executor.submit( + () -> + commit.commit( + Collections.singletonList( + new TwoPhaseCommitMessage(committer)))); + + assertThat(fileIO.awaitFirstWave()).isTrue(); + verify(committer, never()).commit(fileIO); + + fileIO.releaseFirstWave(); + result.get(10, TimeUnit.SECONDS); + assertThat(fileIO.deleteCalls()).isEqualTo(65); + assertThat(fileIO.maxConcurrentDeletes()).isEqualTo(64); + verify(committer).commit(fileIO); + } finally { + fileIO.releaseFirstWave(); + executor.shutdownNow(); + } + } + + @Test + void testCatalogManagedBuilderHonorsConfiguredSerialCleanup() throws Exception { + SerialProbeFileIO fileIO = new SerialProbeFileIO(); + Path tablePath = new Path(tempDir.toUri()); + Path partitionPath = new Path(tablePath, "part=p"); + writeOldFiles(fileIO, partitionPath, 3); + FormatTablePartitionManager partitionManager = mock(FormatTablePartitionManager.class); + TwoPhaseOutputStream.Committer committer = mock(TwoPhaseOutputStream.Committer.class); + when(committer.targetPath()).thenReturn(new Path(partitionPath, "data-new.csv")); + FormatTableCommit commit = + builderOverwriteCommit( + tablePath, + fileIO, + partitionManager, + Collections.singletonMap( + CoreOptions.FORMAT_TABLE_COMMIT_CLEANUP_THREAD_NUM.key(), "1"), + Collections.singletonMap("part", "p")); + + commit.commit(Collections.singletonList(new TwoPhaseCommitMessage(committer))); + + assertThat(fileIO.deleteCalls()).isEqualTo(3); + assertThat(fileIO.maxConcurrentDeletes()).isEqualTo(1); + } + + @Test + void testCatalogManagedBuilderPropagatesConfiguredCleanupConcurrency() throws Exception { + ParallelDeleteFileIO fileIO = new ParallelDeleteFileIO(7, true); + Path tablePath = new Path(tempDir.toUri()); + Path partitionPath = new Path(tablePath, "part=p"); + writeOldFiles(fileIO, partitionPath, 8); + FormatTablePartitionManager partitionManager = mock(FormatTablePartitionManager.class); + FormatTableCommit commit = + builderOverwriteCommit( + tablePath, + fileIO, + partitionManager, + Collections.singletonMap( + CoreOptions.FORMAT_TABLE_COMMIT_CLEANUP_THREAD_NUM.key(), "7"), + Collections.singletonMap("part", "p")); + ExecutorService executor = Executors.newSingleThreadExecutor(); + try { + Future result = executor.submit(() -> commit.commit(Collections.emptyList())); + + assertThat(fileIO.awaitFirstWave()).isTrue(); + assertThat(fileIO.awaitUnexpectedExtraDelete()).isFalse(); + + fileIO.releaseFirstWave(); + result.get(10, TimeUnit.SECONDS); + assertThat(fileIO.deleteCalls()).isEqualTo(8); + assertThat(fileIO.maxConcurrentDeletes()).isEqualTo(7); + } finally { + fileIO.releaseFirstWave(); + executor.shutdownNow(); + } + } + + @Test + void testFilesystemDiscoveredFormatTableCleanupRemainsSerial() throws Exception { + SerialProbeFileIO fileIO = new SerialProbeFileIO(); + Path tablePath = new Path(tempDir.toUri()); + Path partitionPath = new Path(tablePath, "part=p"); + writeOldFiles(fileIO, partitionPath, 3); + TwoPhaseOutputStream.Committer committer = mock(TwoPhaseOutputStream.Committer.class); + when(committer.targetPath()).thenReturn(new Path(partitionPath, "data-new.csv")); + FormatTableCommit commit = + builderOverwriteCommit( + tablePath, + fileIO, + null, + Collections.singletonMap( + CoreOptions.FORMAT_TABLE_COMMIT_CLEANUP_THREAD_NUM.key(), "64"), + Collections.singletonMap("part", "p")); + + commit.commit(Collections.singletonList(new TwoPhaseCommitMessage(committer))); + + assertThat(fileIO.deleteCalls()).isEqualTo(3); + assertThat(fileIO.maxConcurrentDeletes()).isEqualTo(1); + } + + @Test + void testCleanupIsAHardBarrierBeforePublishingNewFiles() throws Exception { + PartialBarrierDeleteFileIO fileIO = new PartialBarrierDeleteFileIO(); + Path tablePath = new Path(tempDir.toUri()); + Path partitionPath = new Path(tablePath, "part=p"); + writeOldFiles(fileIO, partitionPath, 2); + TwoPhaseOutputStream.Committer committer = mock(TwoPhaseOutputStream.Committer.class); + when(committer.targetPath()).thenReturn(new Path(partitionPath, "data-new.csv")); + doAnswer( + invocation -> { + assertThat(fileIO.activeDeletes()).isZero(); + return null; + }) + .when(committer) + .commit(fileIO); + FormatTableCommit commit = + newCleanupCommit(tablePath, fileIO, null, Collections.singletonMap("part", "p"), 2); + ExecutorService executor = Executors.newSingleThreadExecutor(); + try { + Future result = + executor.submit( + () -> + commit.commit( + Collections.singletonList( + new TwoPhaseCommitMessage(committer)))); + + assertThat(fileIO.awaitBothDeletesStarted()).isTrue(); + verify(committer, never()).commit(fileIO); + + fileIO.releaseFirstDelete(); + assertThat(fileIO.awaitFirstDeleteReturned()).isTrue(); + assertThatThrownBy(() -> result.get(300, TimeUnit.MILLISECONDS)) + .isInstanceOf(TimeoutException.class); + verify(committer, never()).commit(fileIO); + + fileIO.releaseSecondDelete(); + result.get(10, TimeUnit.SECONDS); + verify(committer).commit(fileIO); + } finally { + fileIO.releaseFirstDelete(); + fileIO.releaseSecondDelete(); + executor.shutdownNow(); + } + } + + @Test + void testUnpartitionedCatalogManagedFormatTableCleanupRemainsSerial() throws Exception { + SerialProbeFileIO fileIO = new SerialProbeFileIO(); + Path tablePath = new Path(tempDir.toUri()); + writeOldFiles(fileIO, tablePath, 3); + FormatTablePartitionManager partitionManager = mock(FormatTablePartitionManager.class); + TwoPhaseOutputStream.Committer committer = mock(TwoPhaseOutputStream.Committer.class); + when(committer.targetPath()).thenReturn(new Path(tablePath, "data-new.csv")); + FormatTableCommit commit = + builderUnpartitionedOverwriteCommit( + tablePath, + fileIO, + partitionManager, + Collections.singletonMap( + CoreOptions.FORMAT_TABLE_COMMIT_CLEANUP_THREAD_NUM.key(), "64")); + + commit.commit(Collections.singletonList(new TwoPhaseCommitMessage(committer))); + + assertThat(fileIO.deleteCalls()).isEqualTo(3); + assertThat(fileIO.maxConcurrentDeletes()).isEqualTo(1); + } + + @Test + void testCleanupFailureStopsNewSubmissionsAndDrainsTheAlreadyRunningDelete() throws Exception { + FailureDrainFileIO fileIO = new FailureDrainFileIO(); + Path tablePath = new Path(tempDir.toUri()); + Path partitionPath = new Path(tablePath, "part=p"); + writeOldFiles(fileIO, partitionPath, 6); + TwoPhaseOutputStream.Committer committer = mock(TwoPhaseOutputStream.Committer.class); + when(committer.targetPath()).thenReturn(new Path(partitionPath, "data-new.csv")); + FormatTableCommit commit = + newCleanupCommit(tablePath, fileIO, null, Collections.singletonMap("part", "p"), 2); + ExecutorService executor = Executors.newSingleThreadExecutor(); + try { + Future result = + executor.submit( + () -> + commit.commit( + Collections.singletonList( + new TwoPhaseCommitMessage(committer)))); + + assertThat(fileIO.awaitFailureAttempted()).isTrue(); + assertThatThrownBy(() -> result.get(300, TimeUnit.MILLISECONDS)) + .isInstanceOf(TimeoutException.class); + assertThat(fileIO.attemptedFiles()) + .containsExactlyInAnyOrder("data-000.csv", "data-001.csv"); + + fileIO.releaseSuccessfulSibling(); + assertThat(getRootCause(awaitFailure(result))) + .hasMessage("delete failed at input position 0"); + assertThat(fileIO.attemptedFiles()) + .containsExactlyInAnyOrder("data-000.csv", "data-001.csv"); + assertThat(fileIO.successfulFiles()).containsExactly("data-001.csv"); + verify(committer, never()).commit(fileIO); + } finally { + fileIO.releaseSuccessfulSibling(); + executor.shutdownNow(); + } + } + + @Test + void testLaterPartitionListingFailureDrainsAcceptedDeletesBeforeAbort() throws Exception { + Path tablePath = new Path(new Path(tempDir.toUri()), "listing-failure"); + Path firstPartition = new Path(tablePath, "part=p0"); + Path failingPartition = new Path(tablePath, "part=p1"); + LaterRootListingFailureFileIO fileIO = + new LaterRootListingFailureFileIO(failingPartition, 2); + writeOldFiles(fileIO, firstPartition, 2); + writeOldFiles(fileIO, failingPartition, 1); + + AtomicInteger discardCalls = new AtomicInteger(); + ConcurrentLinkedQueue activeDeletesAtDiscard = new ConcurrentLinkedQueue<>(); + List messages = new ArrayList<>(); + List committers = new ArrayList<>(); + for (Path partition : Arrays.asList(firstPartition, failingPartition)) { + TwoPhaseOutputStream.Committer committer = mock(TwoPhaseOutputStream.Committer.class); + when(committer.targetPath()).thenReturn(new Path(partition, "data-new.csv")); + doAnswer( + invocation -> { + activeDeletesAtDiscard.add(fileIO.activeDeletes()); + discardCalls.incrementAndGet(); + return null; + }) + .when(committer) + .discard(fileIO); + committers.add(committer); + messages.add(new TwoPhaseCommitMessage(committer)); + } + FormatTableCommit commit = newCleanupCommit(tablePath, fileIO, null, null, 3); + + ExecutorService executor = Executors.newSingleThreadExecutor(); + Future result = executor.submit(() -> commit.commit(messages)); + try { + assertThat(fileIO.awaitListingFailure()).isTrue(); + assertThatThrownBy(() -> result.get(300, TimeUnit.MILLISECONDS)) + .isInstanceOf(TimeoutException.class); + assertThat(discardCalls).hasValue(0); + for (TwoPhaseOutputStream.Committer committer : committers) { + verify(committer, never()).commit(fileIO); + verify(committer, never()).discard(fileIO); + } + + fileIO.releaseFirstDelete(); + assertThat(fileIO.awaitFirstDeleteReturned()).isTrue(); + assertThatThrownBy(() -> result.get(300, TimeUnit.MILLISECONDS)) + .isInstanceOf(TimeoutException.class); + assertThat(discardCalls).hasValue(0); + + fileIO.releaseSecondDelete(); + assertThat(getRootCause(awaitFailure(result))) + .hasMessage("Failed to list the later partition root."); + assertThat(discardCalls).hasValue(2); + assertThat(activeDeletesAtDiscard).containsExactly(0, 0); + for (TwoPhaseOutputStream.Committer committer : committers) { + verify(committer, never()).commit(fileIO); + verify(committer).discard(fileIO); + } + } finally { + fileIO.releaseFirstDelete(); + fileIO.releaseSecondDelete(); + try { + if (!result.isDone()) { + try { + result.get(10, TimeUnit.SECONDS); + } catch (ExecutionException ignored) { + // The test expects the listing failure above. + } + } + } finally { + executor.shutdownNow(); + } + } + } + + @Test + void testCleanupSelectsLowestInputFailureAndSuppressesTheOtherFailure() throws Exception { + OrderedDualFailureFileIO fileIO = new OrderedDualFailureFileIO(); + Path tablePath = new Path(tempDir.toUri()); + Path partitionPath = new Path(tablePath, "part=p"); + writeOldFiles(fileIO, partitionPath, 2); + FormatTableCommit commit = + newCleanupCommit(tablePath, fileIO, null, Collections.singletonMap("part", "p"), 2); + ExecutorService executor = Executors.newSingleThreadExecutor(); + try { + Future result = executor.submit(() -> commit.commit(Collections.emptyList())); + assertThat(fileIO.awaitHigherPositionFailure()).isTrue(); + assertThatThrownBy(() -> result.get(300, TimeUnit.MILLISECONDS)) + .isInstanceOf(TimeoutException.class); + + fileIO.releaseLowerPositionFailure(); + Throwable primary = getRootCause(awaitFailure(result)); + assertThat(primary).hasMessage("delete failed at input position 0"); + assertThat(primary.getSuppressed()) + .extracting(Throwable::getMessage) + .containsExactly("delete failed at input position 1"); + } finally { + fileIO.releaseLowerPositionFailure(); + executor.shutdownNow(); + } + } + + @Test + void testInterruptDrainsCleanupRestoresFlagAndNeverPublishes() throws Exception { + BlockingDeleteFileIO fileIO = new BlockingDeleteFileIO(2); + Path tablePath = new Path(tempDir.toUri()); + Path partitionPath = new Path(tablePath, "part=p"); + writeOldFiles(fileIO, partitionPath, 2); + TwoPhaseOutputStream.Committer committer = mock(TwoPhaseOutputStream.Committer.class); + when(committer.targetPath()).thenReturn(new Path(partitionPath, "data-new.csv")); + FormatTableCommit commit = + newCleanupCommit(tablePath, fileIO, null, Collections.singletonMap("part", "p"), 2); + AtomicReference failure = new AtomicReference<>(); + AtomicBoolean interruptRestored = new AtomicBoolean(); + CountDownLatch commitReturned = new CountDownLatch(1); + Thread commitThread = + new Thread( + () -> { + try { + commit.commit( + Collections.singletonList( + new TwoPhaseCommitMessage(committer))); + } catch (Throwable t) { + failure.set(t); + } finally { + interruptRestored.set(Thread.currentThread().isInterrupted()); + commitReturned.countDown(); + } + }, + "format-cleanup-interrupted-caller"); + + commitThread.start(); + try { + assertThat(fileIO.awaitDeletesStarted()).isTrue(); + commitThread.interrupt(); + assertThat(commitReturned.await(300, TimeUnit.MILLISECONDS)).isFalse(); + + fileIO.releaseDeletes(); + assertThat(commitReturned.await(10, TimeUnit.SECONDS)).isTrue(); + assertThat(failure.get()).isNotNull(); + assertThat(getCausalChain(failure.get())) + .anyMatch(InterruptedException.class::isInstance); + assertThat(interruptRestored).isTrue(); + verify(committer, never()).commit(fileIO); + } finally { + fileIO.releaseDeletes(); + commitThread.interrupt(); + commitThread.join(TimeUnit.SECONDS.toMillis(10)); + } + assertThat(commitThread.isAlive()).isFalse(); + } + + @SuppressWarnings({"unchecked", "rawtypes"}) + @Test + void testCleanupStatisticsClaimOnlyFilesDeletedByThisCommit() throws Exception { + MixedOwnershipFileIO fileIO = new MixedOwnershipFileIO(); + Path tablePath = new Path(tempDir.toUri()); + writeOldFiles(fileIO, new Path(tablePath, "year=2025/month=00"), 1); + writeOldFiles(fileIO, new Path(tablePath, "year=2025/month=01"), 1); + writeOldFiles(fileIO, new Path(tablePath, "year=2025/month=02"), 1); + FormatTablePartitionManager partitionManager = mock(FormatTablePartitionManager.class); + FormatTableCommit commit = + new FormatTableCommit( + tablePath.toString(), + Arrays.asList("year", "month"), + fileIO, + false, + PARTITION_DEFAULT_NAME.defaultValue(), + true, + Identifier.create("cleanup_db", "cleanup_table"), + Collections.singletonMap("year", "2025"), + null, + null, + partitionManager, + /* dynamicPartitionOverwrite */ true, + 2); + + commit.commit(Collections.emptyList()); + + Map owned = partitionSpec("2025", "00"); + ArgumentCaptor>> specs = + ArgumentCaptor.forClass((Class) List.class); + ArgumentCaptor> statistics = + ArgumentCaptor.forClass((Class) List.class); + verify(partitionManager) + .createPartitions(specs.capture(), eq(true), statistics.capture(), eq(true)); + assertThat(specs.getValue()).containsExactly(owned); + assertThat(statistics.getValue()) + .singleElement() + .satisfies( + stat -> { + assertThat(stat.spec()).isEqualTo(owned); + assertThat(stat.recordCount()).isZero(); + assertThat(stat.fileSizeInBytes()).isZero(); + assertThat(stat.fileCount()).isZero(); + }); + assertThat(fileIO.exists(new Path(tablePath, "year=2025/month=00/data-000.csv"))).isFalse(); + assertThat(fileIO.exists(new Path(tablePath, "year=2025/month=01/data-000.csv"))).isFalse(); + assertThat(fileIO.exists(new Path(tablePath, "year=2025/month=02/data-000.csv"))).isFalse(); + } + + @Test + void testCleanupRejectsFalseWhenTheOldDataFileStillExists() throws Exception { + RefusingDeleteFileIO fileIO = new RefusingDeleteFileIO(); + Path tablePath = new Path(tempDir.toUri()); + Path partitionPath = new Path(tablePath, "part=p"); + writeOldFiles(fileIO, partitionPath, 1); + TwoPhaseOutputStream.Committer committer = mock(TwoPhaseOutputStream.Committer.class); + when(committer.targetPath()).thenReturn(new Path(partitionPath, "data-new.csv")); + FormatTableCommit commit = + newCleanupCommit(tablePath, fileIO, null, Collections.singletonMap("part", "p"), 2); + + assertThatThrownBy( + () -> + commit.commit( + Collections.singletonList( + new TwoPhaseCommitMessage(committer)))) + .hasRootCauseMessage( + "Failed to delete data file " + + new Path(partitionPath, "data-000.csv") + + " of table cleanup_db.cleanup_table."); + verify(committer, never()).commit(fileIO); + verify(committer).discard(fileIO); + } + + @SuppressWarnings({"unchecked", "rawtypes"}) + @Test + void testConcurrentCleanupReportsCompleteStatisticsAfterBarrier() throws Exception { + ParallelDeleteFileIO fileIO = new ParallelDeleteFileIO(4); + Path tablePath = new Path(tempDir.toUri()); + for (int month = 0; month < 8; month++) { + writeOldFiles( + fileIO, new Path(tablePath, String.format("year=2025/month=%02d", month)), 1); + } + FormatTablePartitionManager partitionManager = mock(FormatTablePartitionManager.class); + doAnswer( + invocation -> { + assertThat(fileIO.activeDeletes()).isZero(); + return null; + }) + .when(partitionManager) + .createPartitions(anyList(), eq(true), anyList(), eq(true)); + FormatTableCommit commit = + new FormatTableCommit( + tablePath.toString(), + Arrays.asList("year", "month"), + fileIO, + false, + PARTITION_DEFAULT_NAME.defaultValue(), + true, + Identifier.create("cleanup_db", "cleanup_table"), + Collections.singletonMap("year", "2025"), + null, + null, + partitionManager, + /* dynamicPartitionOverwrite */ true, + 4); + + commit.commit(Collections.emptyList()); + + List> expectedSpecs = new ArrayList<>(); + for (int month = 0; month < 8; month++) { + expectedSpecs.add(partitionSpec("2025", String.format("%02d", month))); + } + ArgumentCaptor>> specs = + ArgumentCaptor.forClass((Class) List.class); + ArgumentCaptor> statistics = + ArgumentCaptor.forClass((Class) List.class); + verify(partitionManager) + .createPartitions(specs.capture(), eq(true), statistics.capture(), eq(true)); + assertThat(specs.getValue()).containsExactlyInAnyOrderElementsOf(expectedSpecs); + assertThat(statistics.getValue()) + .hasSize(8) + .extracting(PartitionStatistics::spec) + .containsExactlyInAnyOrderElementsOf(expectedSpecs); + assertThat(statistics.getValue()) + .allSatisfy( + stat -> { + assertThat(stat.recordCount()).isZero(); + assertThat(stat.fileSizeInBytes()).isZero(); + assertThat(stat.fileCount()).isZero(); + }); + } + + @Test + void testCatalogManagedOverwriteCleanupSpansPartitionDirectories() throws Exception { + assertOverwriteCleanupSpansPartitions(/* dynamicPartitionOverwrite */ true); + assertOverwriteCleanupSpansPartitions(/* dynamicPartitionOverwrite */ false); + } + + @Test + void testCleanupDoesNotListEveryPartitionBeforeTheFirstDeleteWindowCompletes() + throws Exception { + Path tablePath = new Path(new Path(tempDir.toUri()), "lazy-root-listing"); + Path firstPartition = new Path(tablePath, "part=p0"); + Path deferredPartition = new Path(tablePath, "part=p1"); + LazyRootListingFileIO fileIO = new LazyRootListingFileIO(2, deferredPartition); + writeOldFiles(fileIO, firstPartition, 2); + writeOldFiles(fileIO, deferredPartition, 2); + + List messages = new ArrayList<>(); + for (Path partition : Arrays.asList(firstPartition, deferredPartition)) { + TwoPhaseOutputStream.Committer committer = mock(TwoPhaseOutputStream.Committer.class); + when(committer.targetPath()).thenReturn(new Path(partition, "data-new.csv")); + messages.add(new TwoPhaseCommitMessage(committer)); + } + Map options = new LinkedHashMap<>(); + options.put(CoreOptions.FORMAT_TABLE_COMMIT_CLEANUP_THREAD_NUM.key(), "2"); + FormatTableCommit commit = + builderOverwriteCommit( + tablePath, + fileIO, + mock(FormatTablePartitionManager.class), + options, + /* staticPartition */ null); + + ExecutorService executor = Executors.newSingleThreadExecutor(); + Future result = null; + try { + result = executor.submit(() -> commit.commit(messages)); + assertThat(fileIO.awaitFirstWave()).isTrue(); + assertThat(fileIO.deferredRootListed()).isFalse(); + } finally { + fileIO.releaseFirstWave(); + if (result != null) { + result.get(10, TimeUnit.SECONDS); + } + executor.shutdownNow(); + } + + assertThat(fileIO.deferredRootListed()).isTrue(); + assertThat(fileIO.deleteCalls()).isEqualTo(4); + } + + @Test + void testBuilderCleanupConcurrencyDoesNotApplyToTruncateOperations() throws Exception { + SerialProbeFileIO tableFileIO = new SerialProbeFileIO(); + Path tablePath = new Path(new Path(tempDir.toUri()), "truncate-table"); + Path tablePartitionPath = new Path(tablePath, "part=p"); + writeOldFiles(tableFileIO, tablePartitionPath, 3); + FormatTablePartitionManager tableManager = mock(FormatTablePartitionManager.class); + when(tableManager.listPartitions(Collections.emptyMap(), null)) + .thenReturn( + Collections.singletonList( + new Partition( + Collections.singletonMap("part", "p"), + 0, + 0, + 0, + 0, + -1, + false))); + builderTruncateCommit(tablePath, tableFileIO, tableManager).truncateTable(); + + SerialProbeFileIO partitionFileIO = new SerialProbeFileIO(); + Path partitionsPath = new Path(new Path(tempDir.toUri()), "truncate-partitions"); + Path namedPartitionPath = new Path(partitionsPath, "part=p"); + writeOldFiles(partitionFileIO, namedPartitionPath, 3); + FormatTablePartitionManager partitionManager = mock(FormatTablePartitionManager.class); + when(partitionManager.listPartitionsByNames(anyList())) + .thenReturn( + Collections.singletonList( + new Partition( + Collections.singletonMap("part", "p"), + 0, + 0, + 0, + 0, + -1, + false))); + builderTruncateCommit(partitionsPath, partitionFileIO, partitionManager) + .truncatePartitions( + Collections.singletonList(Collections.singletonMap("part", "p"))); + + assertThat(tableFileIO.maxConcurrentDeletes()).isEqualTo(1); + assertThat(partitionFileIO.maxConcurrentDeletes()).isEqualTo(1); + } + + @Test + void testAbortFailureDoesNotReplaceInterruptedCleanupFailure() throws Exception { + BlockingDeleteFileIO fileIO = new BlockingDeleteFileIO(2); + Path tablePath = new Path(new Path(tempDir.toUri()), "abort-failure"); + Path partitionPath = new Path(tablePath, "part=p"); + writeOldFiles(fileIO, partitionPath, 2); + TwoPhaseOutputStream.Committer committer = mock(TwoPhaseOutputStream.Committer.class); + when(committer.targetPath()).thenReturn(new Path(partitionPath, "data-new.csv")); + doThrow(new IOException("discard failed after cleanup interruption")) + .when(committer) + .discard(fileIO); + FormatTableCommit commit = + newCleanupCommit(tablePath, fileIO, null, Collections.singletonMap("part", "p"), 2); + AtomicReference failure = new AtomicReference<>(); + AtomicBoolean interruptRestored = new AtomicBoolean(); + Thread commitThread = + new Thread( + () -> { + try { + commit.commit( + Collections.singletonList( + new TwoPhaseCommitMessage(committer))); + } catch (Throwable t) { + failure.set(t); + } finally { + interruptRestored.set(Thread.currentThread().isInterrupted()); + } + }, + "format-cleanup-abort-failure-caller"); + + commitThread.start(); + try { + assertThat(fileIO.awaitDeletesStarted()).isTrue(); + commitThread.interrupt(); + fileIO.releaseDeletes(); + commitThread.join(TimeUnit.SECONDS.toMillis(10)); + } finally { + fileIO.releaseDeletes(); + commitThread.interrupt(); + commitThread.join(TimeUnit.SECONDS.toMillis(10)); + } + + assertThat(commitThread.isAlive()).isFalse(); + assertThat(failure.get()).isNotNull(); + assertThat(getRootCause(failure.get())).isInstanceOf(InterruptedException.class); + assertThat(failureTree(failure.get())) + .extracting(Throwable::getMessage) + .contains("discard failed after cleanup interruption"); + assertThat(interruptRestored).isTrue(); + verify(committer).discard(fileIO); + verify(committer, never()).commit(fileIO); + } + /** * An overwrite that names no partition: what INSERT OVERWRITE without a PARTITION clause is. */ @@ -652,7 +1344,565 @@ private FormatTableCommit overwritingCommit( null, null, null, - dynamicPartitionOverwrite); + dynamicPartitionOverwrite, + /* cleanupThreadNum */ 1); + } + + private void assertOverwriteCleanupSpansPartitions(boolean dynamicPartitionOverwrite) + throws Exception { + ParallelDeleteFileIO fileIO = new ParallelDeleteFileIO(4); + Path tablePath = + new Path( + new Path(tempDir.toUri()), + dynamicPartitionOverwrite ? "dynamic-roots" : "whole-roots"); + FormatTablePartitionManager partitionManager = mock(FormatTablePartitionManager.class); + List partitions = new ArrayList<>(); + List messages = new ArrayList<>(); + for (int i = 0; i < 4; i++) { + String value = "p" + i; + Path partitionPath = new Path(tablePath, "part=" + value); + writeOldFiles(fileIO, partitionPath, 1); + partitions.add( + new Partition(Collections.singletonMap("part", value), 0, 0, 0, 0, -1, false)); + if (dynamicPartitionOverwrite) { + TwoPhaseOutputStream.Committer committer = + mock(TwoPhaseOutputStream.Committer.class); + when(committer.targetPath()).thenReturn(new Path(partitionPath, "data-new.csv")); + messages.add(new TwoPhaseCommitMessage(committer)); + } + } + if (!dynamicPartitionOverwrite) { + when(partitionManager.listPartitions(Collections.emptyMap(), null)) + .thenReturn(partitions); + } + Map options = new LinkedHashMap<>(); + options.put(CoreOptions.FORMAT_TABLE_COMMIT_CLEANUP_THREAD_NUM.key(), "4"); + options.put( + CoreOptions.DYNAMIC_PARTITION_OVERWRITE.key(), + Boolean.toString(dynamicPartitionOverwrite)); + FormatTableCommit commit = + builderOverwriteCommit( + tablePath, fileIO, partitionManager, options, /* staticPartition */ null); + + commit.commit(messages); + + assertThat(fileIO.deleteCalls()).isEqualTo(4); + assertThat(fileIO.maxConcurrentDeletes()).isEqualTo(4); + } + + private FormatTableCommit builderOverwriteCommit( + Path tablePath, + FileIO fileIO, + FormatTablePartitionManager partitionManager, + Map options, + Map staticPartition) { + FormatTable table = formatTable(tablePath, fileIO, partitionManager, options); + BatchWriteBuilder writeBuilder = table.newBatchWriteBuilder(); + writeBuilder.withOverwrite(staticPartition); + return (FormatTableCommit) writeBuilder.newCommit(); + } + + private FormatTableCommit builderTruncateCommit( + Path tablePath, FileIO fileIO, FormatTablePartitionManager partitionManager) { + return (FormatTableCommit) + formatTable(tablePath, fileIO, partitionManager, Collections.emptyMap()) + .newBatchWriteBuilder() + .newCommit(); + } + + private FormatTable formatTable( + Path tablePath, + FileIO fileIO, + FormatTablePartitionManager partitionManager, + Map options) { + RowType rowType = + RowType.builder() + .field("part", DataTypes.STRING()) + .field("id", DataTypes.INT()) + .build(); + FormatTable table = + FormatTable.builder() + .fileIO(fileIO) + .identifier(Identifier.create("cleanup_db", "cleanup_table")) + .rowType(rowType) + .partitionKeys(Collections.singletonList("part")) + .location(tablePath.toString()) + .format(FormatTable.Format.CSV) + .options(options) + .partitionManager(partitionManager) + .build(); + return table; + } + + private FormatTableCommit builderUnpartitionedOverwriteCommit( + Path tablePath, + FileIO fileIO, + FormatTablePartitionManager partitionManager, + Map options) { + FormatTable table = + FormatTable.builder() + .fileIO(fileIO) + .identifier(Identifier.create("cleanup_db", "cleanup_table")) + .rowType(RowType.builder().field("id", DataTypes.INT()).build()) + .partitionKeys(Collections.emptyList()) + .location(tablePath.toString()) + .format(FormatTable.Format.CSV) + .options(options) + .partitionManager(partitionManager) + .build(); + BatchWriteBuilder writeBuilder = table.newBatchWriteBuilder(); + writeBuilder.withOverwrite(null); + return (FormatTableCommit) writeBuilder.newCommit(); + } + + private FormatTableCommit newCleanupCommit( + Path tablePath, + FileIO fileIO, + FormatTablePartitionManager partitionManager, + Map staticPartition, + int cleanupThreadNum) { + return new FormatTableCommit( + tablePath.toString(), + Collections.singletonList("part"), + fileIO, + false, + PARTITION_DEFAULT_NAME.defaultValue(), + true, + Identifier.create("cleanup_db", "cleanup_table"), + staticPartition, + null, + null, + partitionManager, + /* dynamicPartitionOverwrite */ true, + cleanupThreadNum); + } + + private static void writeOldFiles(LocalFileIO fileIO, Path partitionPath, int count) + throws IOException { + for (int i = 0; i < count; i++) { + fileIO.writeFile( + new Path(partitionPath, String.format("data-%03d.csv", i)), "old", false); + } + } + + private static ExecutionException awaitFailure(Future future) throws Exception { + try { + future.get(10, TimeUnit.SECONDS); + throw new AssertionError("Expected cleanup commit to fail"); + } catch (ExecutionException expected) { + return expected; + } + } + + private static List failureTree(Throwable throwable) { + List failures = new ArrayList<>(); + collectFailures(throwable, failures); + return failures; + } + + private static void collectFailures(Throwable throwable, List failures) { + if (throwable == null) { + return; + } + failures.add(throwable); + for (Throwable suppressed : throwable.getSuppressed()) { + collectFailures(suppressed, failures); + } + collectFailures(throwable.getCause(), failures); + } + + private static class ParallelDeleteFileIO extends LocalFileIO { + + private final int firstWaveSize; + private final boolean holdFirstWave; + private final CountDownLatch firstWave; + private final CountDownLatch releaseFirstWave = new CountDownLatch(1); + private final CountDownLatch unexpectedExtraDelete = new CountDownLatch(1); + private final AtomicInteger deleteCalls = new AtomicInteger(); + private final AtomicInteger activeDeletes = new AtomicInteger(); + private final AtomicInteger maxConcurrentDeletes = new AtomicInteger(); + + private ParallelDeleteFileIO(int firstWaveSize) { + this(firstWaveSize, false); + } + + private ParallelDeleteFileIO(int firstWaveSize, boolean holdFirstWave) { + this.firstWaveSize = firstWaveSize; + this.holdFirstWave = holdFirstWave; + this.firstWave = new CountDownLatch(firstWaveSize); + } + + @Override + public boolean delete(Path path, boolean recursive) throws IOException { + int call = deleteCalls.incrementAndGet(); + int active = activeDeletes.incrementAndGet(); + maxConcurrentDeletes.updateAndGet(previous -> Math.max(previous, active)); + try { + if (call <= firstWaveSize) { + firstWave.countDown(); + if (!firstWave.await(10, TimeUnit.SECONDS)) { + throw new IOException("Expected cleanup delete calls did not overlap"); + } + if (holdFirstWave && !releaseFirstWave.await(10, TimeUnit.SECONDS)) { + throw new IOException("Test did not release the first cleanup wave"); + } + } else { + unexpectedExtraDelete.countDown(); + } + return super.delete(path, recursive); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IOException("Interrupted while observing cleanup concurrency", e); + } finally { + activeDeletes.decrementAndGet(); + } + } + + protected int deleteCalls() { + return deleteCalls.get(); + } + + private int maxConcurrentDeletes() { + return maxConcurrentDeletes.get(); + } + + private int activeDeletes() { + return activeDeletes.get(); + } + + protected boolean awaitFirstWave() throws InterruptedException { + return firstWave.await(10, TimeUnit.SECONDS); + } + + private boolean awaitUnexpectedExtraDelete() throws InterruptedException { + return unexpectedExtraDelete.await(300, TimeUnit.MILLISECONDS); + } + + protected void releaseFirstWave() { + releaseFirstWave.countDown(); + } + } + + private static class LazyRootListingFileIO extends ParallelDeleteFileIO { + + private final Path deferredRoot; + private final AtomicBoolean deferredRootListed = new AtomicBoolean(); + + private LazyRootListingFileIO(int firstWaveSize, Path deferredRoot) { + super(firstWaveSize, true); + this.deferredRoot = deferredRoot; + } + + @Override + public FileStatus[] listStatus(Path path) throws IOException { + if (deferredRoot.equals(path)) { + deferredRootListed.set(true); + } + return super.listStatus(path); + } + + private boolean deferredRootListed() { + return deferredRootListed.get(); + } + } + + private static class SerialProbeFileIO extends LocalFileIO { + + private final CountDownLatch secondDeleteStarted = new CountDownLatch(1); + private final AtomicInteger deleteCalls = new AtomicInteger(); + private final AtomicInteger activeDeletes = new AtomicInteger(); + private final AtomicInteger maxConcurrentDeletes = new AtomicInteger(); + + @Override + public boolean delete(Path path, boolean recursive) throws IOException { + int call = deleteCalls.incrementAndGet(); + int active = activeDeletes.incrementAndGet(); + maxConcurrentDeletes.updateAndGet(previous -> Math.max(previous, active)); + try { + if (call == 1) { + secondDeleteStarted.await(300, TimeUnit.MILLISECONDS); + } else { + secondDeleteStarted.countDown(); + } + return super.delete(path, recursive); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IOException("Interrupted while probing serial cleanup", e); + } finally { + activeDeletes.decrementAndGet(); + } + } + + private int deleteCalls() { + return deleteCalls.get(); + } + + private int maxConcurrentDeletes() { + return maxConcurrentDeletes.get(); + } + } + + private static class PartialBarrierDeleteFileIO extends SortedLocalFileIO { + + private final CountDownLatch bothDeletesStarted = new CountDownLatch(2); + private final CountDownLatch releaseFirstDelete = new CountDownLatch(1); + private final CountDownLatch releaseSecondDelete = new CountDownLatch(1); + private final CountDownLatch firstDeleteReturned = new CountDownLatch(1); + private final AtomicInteger activeDeletes = new AtomicInteger(); + + @Override + public boolean delete(Path path, boolean recursive) throws IOException { + activeDeletes.incrementAndGet(); + bothDeletesStarted.countDown(); + await(bothDeletesStarted, "both barrier delete calls"); + try { + if ("data-000.csv".equals(path.getName())) { + await(releaseFirstDelete, "first barrier delete release"); + return super.delete(path, recursive); + } + await(releaseSecondDelete, "second barrier delete release"); + return super.delete(path, recursive); + } finally { + activeDeletes.decrementAndGet(); + if ("data-000.csv".equals(path.getName())) { + firstDeleteReturned.countDown(); + } + } + } + + private boolean awaitBothDeletesStarted() throws InterruptedException { + return bothDeletesStarted.await(10, TimeUnit.SECONDS); + } + + private void releaseFirstDelete() { + releaseFirstDelete.countDown(); + } + + private void releaseSecondDelete() { + releaseSecondDelete.countDown(); + } + + private boolean awaitFirstDeleteReturned() throws InterruptedException { + return firstDeleteReturned.await(10, TimeUnit.SECONDS); + } + + private int activeDeletes() { + return activeDeletes.get(); + } + } + + private static class BlockingDeleteFileIO extends LocalFileIO { + + private final CountDownLatch deletesStarted; + private final CountDownLatch releaseDeletes = new CountDownLatch(1); + + private BlockingDeleteFileIO(int deleteCount) { + this.deletesStarted = new CountDownLatch(deleteCount); + } + + @Override + public boolean delete(Path path, boolean recursive) throws IOException { + deletesStarted.countDown(); + try { + if (!releaseDeletes.await(10, TimeUnit.SECONDS)) { + throw new IOException("Test did not release blocked cleanup deletes"); + } + return super.delete(path, recursive); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IOException("Interrupted while blocking cleanup delete", e); + } + } + + private boolean awaitDeletesStarted() throws InterruptedException { + return deletesStarted.await(10, TimeUnit.SECONDS); + } + + private void releaseDeletes() { + releaseDeletes.countDown(); + } + } + + private abstract static class SortedLocalFileIO extends LocalFileIO { + + @Override + public FileStatus[] listStatus(Path path) throws IOException { + FileStatus[] statuses = super.listStatus(path); + Arrays.sort(statuses, Comparator.comparing(status -> status.getPath().toString())); + return statuses; + } + + protected static void await(CountDownLatch latch, String description) throws IOException { + try { + if (!latch.await(10, TimeUnit.SECONDS)) { + throw new IOException("Timed out waiting for " + description); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IOException("Interrupted while waiting for " + description, e); + } + } + } + + private static class LaterRootListingFailureFileIO extends SortedLocalFileIO { + + private final Path failingRoot; + private final CountDownLatch deletesStarted; + private final CountDownLatch listingFailure = new CountDownLatch(1); + private final CountDownLatch releaseFirstDelete = new CountDownLatch(1); + private final CountDownLatch releaseSecondDelete = new CountDownLatch(1); + private final CountDownLatch firstDeleteReturned = new CountDownLatch(1); + private final AtomicInteger activeDeletes = new AtomicInteger(); + + private LaterRootListingFailureFileIO(Path failingRoot, int deleteCount) { + this.failingRoot = failingRoot; + this.deletesStarted = new CountDownLatch(deleteCount); + } + + @Override + public FileStatus[] listStatus(Path path) throws IOException { + if (failingRoot.equals(path)) { + await(deletesStarted, "accepted deletes before later-root listing failure"); + listingFailure.countDown(); + throw new IOException("Failed to list the later partition root."); + } + return super.listStatus(path); + } + + @Override + public boolean delete(Path path, boolean recursive) throws IOException { + activeDeletes.incrementAndGet(); + deletesStarted.countDown(); + try { + if ("data-000.csv".equals(path.getName())) { + await(releaseFirstDelete, "release of the first accepted delete"); + } else { + await(releaseSecondDelete, "release of the second accepted delete"); + } + return super.delete(path, recursive); + } finally { + activeDeletes.decrementAndGet(); + if ("data-000.csv".equals(path.getName())) { + firstDeleteReturned.countDown(); + } + } + } + + private boolean awaitListingFailure() throws InterruptedException { + return listingFailure.await(10, TimeUnit.SECONDS); + } + + private void releaseFirstDelete() { + releaseFirstDelete.countDown(); + } + + private boolean awaitFirstDeleteReturned() throws InterruptedException { + return firstDeleteReturned.await(10, TimeUnit.SECONDS); + } + + private void releaseSecondDelete() { + releaseSecondDelete.countDown(); + } + + private int activeDeletes() { + return activeDeletes.get(); + } + } + + private static class FailureDrainFileIO extends SortedLocalFileIO { + + private final CountDownLatch firstPairStarted = new CountDownLatch(2); + private final CountDownLatch failureAttempted = new CountDownLatch(1); + private final CountDownLatch releaseSuccessfulSibling = new CountDownLatch(1); + private final ConcurrentLinkedQueue attemptedFiles = new ConcurrentLinkedQueue<>(); + private final ConcurrentLinkedQueue successfulFiles = new ConcurrentLinkedQueue<>(); + + @Override + public boolean delete(Path path, boolean recursive) throws IOException { + String name = path.getName(); + attemptedFiles.add(name); + if ("data-000.csv".equals(name)) { + firstPairStarted.countDown(); + await(firstPairStarted, "both initial deletes to start"); + failureAttempted.countDown(); + throw new IOException("delete failed at input position 0"); + } + if ("data-001.csv".equals(name)) { + firstPairStarted.countDown(); + await(firstPairStarted, "both initial deletes to start"); + await(releaseSuccessfulSibling, "release of in-flight sibling"); + boolean deleted = super.delete(path, recursive); + successfulFiles.add(name); + return deleted; + } + return super.delete(path, recursive); + } + + private boolean awaitFailureAttempted() throws InterruptedException { + return failureAttempted.await(10, TimeUnit.SECONDS); + } + + private void releaseSuccessfulSibling() { + releaseSuccessfulSibling.countDown(); + } + + private ConcurrentLinkedQueue attemptedFiles() { + return attemptedFiles; + } + + private ConcurrentLinkedQueue successfulFiles() { + return successfulFiles; + } + } + + private static class OrderedDualFailureFileIO extends SortedLocalFileIO { + + private final CountDownLatch firstPairStarted = new CountDownLatch(2); + private final CountDownLatch higherPositionFailure = new CountDownLatch(1); + private final CountDownLatch releaseLowerPositionFailure = new CountDownLatch(1); + + @Override + public boolean delete(Path path, boolean recursive) throws IOException { + firstPairStarted.countDown(); + await(firstPairStarted, "both failing deletes to start"); + if ("data-001.csv".equals(path.getName())) { + higherPositionFailure.countDown(); + throw new IOException("delete failed at input position 1"); + } + await(releaseLowerPositionFailure, "lower-position failure"); + throw new IOException("delete failed at input position 0"); + } + + private boolean awaitHigherPositionFailure() throws InterruptedException { + return higherPositionFailure.await(10, TimeUnit.SECONDS); + } + + private void releaseLowerPositionFailure() { + releaseLowerPositionFailure.countDown(); + } + } + + private static class MixedOwnershipFileIO extends SortedLocalFileIO { + + @Override + public boolean delete(Path path, boolean recursive) throws IOException { + boolean deleted = super.delete(path, recursive); + if (path.toString().contains("month=01")) { + throw new FileNotFoundException("concurrently deleted " + path); + } + if (path.toString().contains("month=02")) { + return false; + } + return deleted; + } + } + + private static class RefusingDeleteFileIO extends LocalFileIO { + + @Override + public boolean delete(Path path, boolean recursive) { + return false; + } } private static Map partitionSpec(String year, String month) { @@ -681,7 +1931,8 @@ private FormatTableCommit truncatingCommit( null, null, partitionManager, - /* dynamicPartitionOverwrite */ true); + /* dynamicPartitionOverwrite */ true, + /* cleanupThreadNum */ 1); } private FormatTablePartitionManager commitPartitionedFile( @@ -706,7 +1957,8 @@ private FormatTablePartitionManager commitPartitionedFile( null, null, partitionManager, - /* dynamicPartitionOverwrite */ true); + /* dynamicPartitionOverwrite */ true, + /* cleanupThreadNum */ 1); commit.commit(Collections.singletonList(new TwoPhaseCommitMessage(committer))); return partitionManager; } From 29d85583cc7dc88fbe3cf9b40df490116a1aee0b Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Thu, 27 Aug 2026 02:11:15 +0800 Subject: [PATCH 02/10] [core] Parallelize format table file publication Publishing the files a Format Table commit wrote is the other half of the same problem: one synchronous multipart completion per file, on the driver, after cleanup has finished. Cleanup concurrency alone leaves that untouched. Publish through the same bounded runner, under its own format-table.commit.publish-thread-num with the same scope and the same opt out. The caller still publishes directly when there is one file or one thread, so a small commit gains nothing and risks nothing. Statistics, staging clean up and the catalog update stay on the caller, after every publish has been waited for. Nothing reads a partition's files while another thread may still be adding to them. Roll a failed commit back file by file. A publication that a concurrent commit can fail part way through leaves files behind that no partition should hold, and discarding the staging output does not remove one that was already published. Every target belongs to this write attempt, so removing it is safe even when a completion took effect but its response was lost. Carry the caller's access control context into the workers, so publication runs with the same permissions whether or not it is handed to the pool. --- docs/generated/core_configuration.html | 6 + .../java/org/apache/paimon/CoreOptions.java | 20 ++ .../apache/paimon/utils/ThreadPoolUtils.java | 18 +- .../paimon/utils/ThreadPoolUtilsTest.java | 51 +++ .../table/format/FormatBatchWriteBuilder.java | 7 +- .../table/format/FormatTableCommit.java | 163 +++++++-- .../org/apache/paimon/CoreOptionsTest.java | 25 ++ .../table/format/FormatTableCommitTest.java | 327 +++++++++++++++++- 8 files changed, 591 insertions(+), 26 deletions(-) diff --git a/docs/generated/core_configuration.html b/docs/generated/core_configuration.html index e544bec90fb5..4dca9eb1d6b7 100644 --- a/docs/generated/core_configuration.html +++ b/docs/generated/core_configuration.html @@ -752,6 +752,12 @@ Integer The maximum number of concurrent deletions of old data files during overwrite commits for an internal Format Table with catalog-managed partitions. Supported values are 1 through 64. Other Format Tables use serial cleanup. This limit uses a separate thread pool and is independent of file-operation.thread-num, so the total file-operation concurrency in one process may be the sum of both limits. + +

format-table.commit.publish-thread-num
+ 64 + Integer + The maximum number of concurrent file publications during commits for a partitioned Format Table with catalog-managed partitions. Supported values are 1 through 64. Other Format Tables publish serially. +
format-table.file.compression
(none) diff --git a/paimon-api/src/main/java/org/apache/paimon/CoreOptions.java b/paimon-api/src/main/java/org/apache/paimon/CoreOptions.java index 363aec22b29e..9d2b2c6b5927 100644 --- a/paimon-api/src/main/java/org/apache/paimon/CoreOptions.java +++ b/paimon-api/src/main/java/org/apache/paimon/CoreOptions.java @@ -2667,6 +2667,16 @@ public String toString() { + "file-operation.thread-num, so the total file-operation " + "concurrency in one process may be the sum of both limits."); + public static final ConfigOption FORMAT_TABLE_COMMIT_PUBLISH_THREAD_NUM = + key("format-table.commit.publish-thread-num") + .intType() + .defaultValue(64) + .withDescription( + "The maximum number of concurrent file publications during commits " + + "for a partitioned Format Table with catalog-managed " + + "partitions. Supported values are 1 through 64. Other Format " + + "Tables publish serially."); + @Immutable public static final ConfigOption BLOB_FIELD = key("blob-field") @@ -3325,6 +3335,16 @@ public int formatTableCommitCleanupThreadNum() { return threadNum; } + public int formatTableCommitPublishThreadNum() { + int threadNum = options.get(FORMAT_TABLE_COMMIT_PUBLISH_THREAD_NUM); + checkArgument( + threadNum >= 1 && threadNum <= 64, + "Option %s must be between 1 and 64, but was %s.", + FORMAT_TABLE_COMMIT_PUBLISH_THREAD_NUM.key(), + threadNum); + return threadNum; + } + public MemorySize fileReaderAsyncThreshold() { return options.get(FILE_READER_ASYNC_THRESHOLD); } diff --git a/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java b/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java index e91ba169e304..37db694e93c3 100644 --- a/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java +++ b/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java @@ -21,6 +21,9 @@ import org.apache.paimon.shade.guava30.com.google.common.collect.Iterators; import org.apache.paimon.shade.guava30.com.google.common.collect.Lists; +import java.security.AccessControlContext; +import java.security.AccessController; +import java.security.PrivilegedAction; import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collection; @@ -319,6 +322,7 @@ private void advanceIfNeeded() { /** Does not consume more input than the active-task window can hold. */ private void fillWindow() { ClassLoader classLoader = Thread.currentThread().getContextClassLoader(); + AccessControlContext accessControlContext = AccessController.getContext(); while (activeTasks.size() < queueSize) { synchronized (submissionLock) { if (submissionStopped || !input.hasNext()) { @@ -326,7 +330,11 @@ private void fillWindow() { } BatchTask task = new BatchTask<>( - processor, input.next(), classLoader, this::stopSubmission); + processor, + input.next(), + classLoader, + accessControlContext, + this::stopSubmission); executor.execute(task); activeTasks.add(task); } @@ -401,6 +409,7 @@ private static class BatchTask implements Runnable { private final Function> processor; private final U input; private final ClassLoader classLoader; + private final AccessControlContext accessControlContext; private final Runnable stopSubmission; private final CountDownLatch completion = new CountDownLatch(1); @@ -414,10 +423,12 @@ private BatchTask( Function> processor, U input, ClassLoader classLoader, + AccessControlContext accessControlContext, Runnable stopSubmission) { this.processor = processor; this.input = input; this.classLoader = classLoader; + this.accessControlContext = accessControlContext; this.stopSubmission = stopSubmission; } @@ -438,7 +449,10 @@ public void run() { ClassLoader originalClassLoader = currentThread.getContextClassLoader(); try { currentThread.setContextClassLoader(classLoader); - result = processor.apply(input); + result = + AccessController.doPrivileged( + (PrivilegedAction>) () -> processor.apply(input), + accessControlContext); } catch (RuntimeException | Error taskFailure) { failure = taskFailure; stopSubmission.run(); diff --git a/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java b/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java index 899c7842fffc..b61593482f0e 100644 --- a/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java +++ b/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java @@ -25,6 +25,10 @@ import org.junit.jupiter.api.Test; +import javax.security.auth.Subject; + +import java.security.AccessController; +import java.security.PrivilegedAction; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -267,6 +271,53 @@ public void testDirectExecutorPreservesCallerInterrupt() { } } + @Test + public void testWorkerRunsWithTheSubmittingSubject() throws Exception { + ExecutorService workers = + ThreadPoolUtils.createCachedThreadPool(1, "subject-propagation-test"); + Subject firstSubject = new Subject(); + Subject secondSubject = new Subject(); + List seenSubjects = new ArrayList<>(); + List seenWorkers = new ArrayList<>(); + + try { + for (Subject subject : Arrays.asList(firstSubject, secondSubject)) { + seenSubjects.add( + Subject.doAs( + subject, + (PrivilegedAction) + () -> { + try (CloseableBatchIterator iterator = + ThreadPoolUtils + .sequentialBatchedExecuteCloseable( + workers, + ignored -> { + seenWorkers.add( + Thread + .currentThread()); + return Collections + .singletonList( + Subject + .getSubject( + AccessController + .getContext())); + }, + Collections.singletonList(0), + 1)) { + return iterator.next(); + } + })); + } + + assertThat(seenWorkers.get(1)).isSameAs(seenWorkers.get(0)); + assertThat(seenSubjects.get(0)).isSameAs(firstSubject); + assertThat(seenSubjects.get(1)).isSameAs(secondSubject); + } finally { + workers.shutdownNow(); + assertThat(workers.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); + } + } + @Test public void testCloseCancelsQueuedTasksAndWaitsUninterruptibly() throws Exception { LinkedBlockingQueue taskQueue = new LinkedBlockingQueue<>(); diff --git a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatBatchWriteBuilder.java b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatBatchWriteBuilder.java index 4c8beca9c175..f9d08a5c1bfb 100644 --- a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatBatchWriteBuilder.java +++ b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatBatchWriteBuilder.java @@ -82,6 +82,10 @@ public BatchTableCommit newCommit() { table.partitionManager() != null && !table.partitionKeys().isEmpty() ? options.formatTableCommitCleanupThreadNum() : 1; + int publishThreadNum = + table.partitionManager() != null && !table.partitionKeys().isEmpty() + ? options.formatTableCommitPublishThreadNum() + : 1; return new FormatTableCommit( table.location(), table.partitionKeys(), @@ -95,7 +99,8 @@ public BatchTableCommit newCommit() { table.catalogContext(), table.partitionManager(), options.dynamicPartitionOverwrite(), - cleanupThreadNum); + cleanupThreadNum, + publishThreadNum); } @Override diff --git a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java index e54f67b557f1..f1442d85b0d6 100644 --- a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java +++ b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java @@ -64,6 +64,7 @@ import java.util.stream.Collectors; import static org.apache.paimon.table.format.FormatBatchWriteBuilder.validateStaticPartition; +import static org.apache.paimon.utils.ExceptionUtils.firstOrSuppressed; import static org.apache.paimon.utils.ThreadPoolUtils.CloseableBatchIterator; import static org.apache.paimon.utils.ThreadPoolUtils.sequentialBatchedExecuteAwaitRunningTasksOnClose; @@ -72,11 +73,11 @@ public class FormatTableCommit implements BatchTableCommit { private static final Logger LOG = LoggerFactory.getLogger(FormatTableCommit.class); - private static final int MAX_CLEANUP_THREAD_NUM = 64; + private static final int MAX_COMMIT_THREAD_NUM = 64; - private static final ExecutorService CLEANUP_EXECUTOR = + private static final ExecutorService COMMIT_EXECUTOR = ThreadPoolUtils.createCachedThreadPool( - MAX_CLEANUP_THREAD_NUM, "FORMAT-TABLE-COMMIT-CLEANUP-THREAD-POOL"); + MAX_COMMIT_THREAD_NUM, "FORMAT-TABLE-COMMIT-THREAD-POOL"); private String location; private final boolean formatTablePartitionOnlyValueInPath; @@ -90,6 +91,7 @@ public class FormatTableCommit implements BatchTableCommit { @Nullable private final FormatTablePartitionManager partitionManager; private final boolean dynamicPartitionOverwrite; private final int cleanupThreadNum; + private final int publishThreadNum; public FormatTableCommit( String location, @@ -105,11 +107,49 @@ public FormatTableCommit( @Nullable FormatTablePartitionManager partitionManager, boolean dynamicPartitionOverwrite, int cleanupThreadNum) { - if (cleanupThreadNum < 1 || cleanupThreadNum > MAX_CLEANUP_THREAD_NUM) { + this( + location, + partitionKeys, + fileIO, + formatTablePartitionOnlyValueInPath, + defaultPartName, + overwrite, + tableIdentifier, + staticPartitions, + syncHiveUri, + catalogContext, + partitionManager, + dynamicPartitionOverwrite, + cleanupThreadNum, + 1); + } + + FormatTableCommit( + String location, + List partitionKeys, + FileIO fileIO, + boolean formatTablePartitionOnlyValueInPath, + String defaultPartName, + boolean overwrite, + Identifier tableIdentifier, + @Nullable Map staticPartitions, + @Nullable String syncHiveUri, + CatalogContext catalogContext, + @Nullable FormatTablePartitionManager partitionManager, + boolean dynamicPartitionOverwrite, + int cleanupThreadNum, + int publishThreadNum) { + if (cleanupThreadNum < 1 || cleanupThreadNum > MAX_COMMIT_THREAD_NUM) { throw new IllegalArgumentException( String.format( "Format Table cleanup thread number must be between 1 and %s, but was %s.", - MAX_CLEANUP_THREAD_NUM, cleanupThreadNum)); + MAX_COMMIT_THREAD_NUM, cleanupThreadNum)); + } + if (publishThreadNum < 1 || publishThreadNum > MAX_COMMIT_THREAD_NUM) { + throw new IllegalArgumentException( + String.format( + "Format Table publish thread number must be between 1 and %s, but was %s.", + MAX_COMMIT_THREAD_NUM, publishThreadNum)); } this.location = location; this.fileIO = fileIO; @@ -123,6 +163,7 @@ public FormatTableCommit( this.partitionManager = partitionManager; this.dynamicPartitionOverwrite = dynamicPartitionOverwrite; this.cleanupThreadNum = cleanupThreadNum; + this.publishThreadNum = publishThreadNum; if (syncHiveUri != null) { try { Options options = new Options(); @@ -209,9 +250,9 @@ public void commit(List commitMessages) { boolean reportsStatistics = registersPartitions && partitionManager != null; Map, PartitionStatistics> statisticsByPartition = new LinkedHashMap<>(); + publishMessages(messages); for (TwoPhaseCommitMessage message : messages) { TwoPhaseOutputStream.Committer committer = message.getCommitter(); - committer.commit(this.fileIO); if (registersPartitions) { // Extracted once: registration and statistics must key on the same spec. Map spec = @@ -289,6 +330,37 @@ public void commit(List commitMessages) { } } + private void publishMessages(List messages) throws IOException { + if (publishThreadNum == 1 || messages.size() <= 1) { + for (TwoPhaseCommitMessage message : messages) { + message.getCommitter().commit(fileIO); + } + return; + } + + try (CloseableBatchIterator published = + sequentialBatchedExecuteAwaitRunningTasksOnClose( + COMMIT_EXECUTOR, + this::publishMessage, + messages.iterator(), + publishThreadNum)) { + while (published.hasNext()) { + published.next(); + } + } catch (UncheckedIOException e) { + throw (IOException) unwrapUncheckedIOException(e); + } + } + + private List publishMessage(TwoPhaseCommitMessage message) { + try { + message.getCommitter().commit(fileIO); + return Collections.emptyList(); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + /** * Registers the partitions this commit touched, carrying the statistics of what it wrote. An * overwrite also empties partitions it writes nothing to - those below a static prefix, and @@ -465,20 +537,69 @@ private static Path buildPartitionPath( @Override public void abort(List commitMessages) { - try { - for (CommitMessage commitMessage : commitMessages) { - if (commitMessage instanceof TwoPhaseCommitMessage) { - TwoPhaseCommitMessage twoPhaseCommitMessage = - (TwoPhaseCommitMessage) commitMessage; - twoPhaseCommitMessage.getCommitter().discard(this.fileIO); - } else { - throw new RuntimeException( - "Unsupported commit message type: " - + commitMessage.getClass().getName()); - } + Throwable failure = null; + for (CommitMessage commitMessage : commitMessages) { + if (!(commitMessage instanceof TwoPhaseCommitMessage)) { + failure = + firstOrSuppressed( + new RuntimeException( + "Unsupported commit message type: " + + commitMessage.getClass().getName()), + failure); + continue; + } + + TwoPhaseOutputStream.Committer committer = + ((TwoPhaseCommitMessage) commitMessage).getCommitter(); + try { + committer.discard(fileIO); + } catch (Throwable discardFailure) { + failure = firstOrSuppressed(discardFailure, failure); } - } catch (Exception e) { - throw new RuntimeException(e); + + // FormatTableSingleFileWriter opens every target with overwrite=false. The target is + // therefore owned by this write attempt, so it is safe to remove even when a remote + // multipart completion took effect but its response was lost. Keep this rollback here: + // a generic multipart committer may also be used to overwrite an existing object. + try { + deletePublishedFile(committer.targetPath()); + } catch (Throwable deleteFailure) { + failure = firstOrSuppressed(deleteFailure, failure); + } + } + + if (failure instanceof Error) { + throw (Error) failure; + } + if (failure != null) { + throw new RuntimeException(failure); + } + } + + private void deletePublishedFile(Path targetPath) throws IOException { + String failureMessage = "Failed to delete published Format Table file " + targetPath; + boolean deleted; + try { + deleted = fileIO.delete(targetPath, false); + } catch (FileNotFoundException ignored) { + return; + } catch (IOException e) { + throw new IOException(failureMessage, e); + } + if (deleted) { + return; + } + + boolean stillExists; + try { + stillExists = fileIO.exists(targetPath); + } catch (FileNotFoundException ignored) { + return; + } catch (IOException e) { + throw new IOException(failureMessage, e); + } + if (stillExists) { + throw new IOException(failureMessage); } } @@ -569,7 +690,7 @@ private Set deletePreviousDataFiles( // so a failure cannot leave a worker still deleting after this method returns. try (CloseableBatchIterator cleared = sequentialBatchedExecuteAwaitRunningTasksOnClose( - CLEANUP_EXECUTOR, this::deleteAndReportCleared, dataFiles, threadNum)) { + COMMIT_EXECUTOR, this::deleteAndReportCleared, dataFiles, threadNum)) { while (cleared.hasNext()) { clearedPartitionPaths.add(cleared.next()); } @@ -592,7 +713,7 @@ private static Throwable unwrapUncheckedIOException(Throwable failure) { return unwrapped; } - /** Deletes one file and reports the partition it emptied, for a worker that cannot throw. */ + /** Deletes one listed file and reports its parent when this commit removed the file. */ private List deleteAndReportCleared(FileStatus file) { try { return deleteDataFile(file) diff --git a/paimon-core/src/test/java/org/apache/paimon/CoreOptionsTest.java b/paimon-core/src/test/java/org/apache/paimon/CoreOptionsTest.java index f3e81179d733..ea7513208702 100644 --- a/paimon-core/src/test/java/org/apache/paimon/CoreOptionsTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/CoreOptionsTest.java @@ -325,4 +325,29 @@ public void testFormatTableCommitCleanupThreadNumRejectsValuesOutsideSupportedRa .hasMessageContaining("1") .hasMessageContaining("64"); } + + @Test + public void testFormatTableCommitPublishThreadNumDefaultsTo64AndAcceptsBounds() { + Options conf = new Options(); + assertThat(new CoreOptions(conf).formatTableCommitPublishThreadNum()).isEqualTo(64); + + conf.set(CoreOptions.FORMAT_TABLE_COMMIT_PUBLISH_THREAD_NUM, 1); + assertThat(new CoreOptions(conf).formatTableCommitPublishThreadNum()).isEqualTo(1); + + conf.set(CoreOptions.FORMAT_TABLE_COMMIT_PUBLISH_THREAD_NUM, 64); + assertThat(new CoreOptions(conf).formatTableCommitPublishThreadNum()).isEqualTo(64); + } + + @Test + public void testFormatTableCommitPublishThreadNumRejectsValuesOutsideSupportedRange() { + for (int invalid : new int[] {0, -1, 65}) { + Options conf = new Options(); + conf.set(CoreOptions.FORMAT_TABLE_COMMIT_PUBLISH_THREAD_NUM, invalid); + assertThatThrownBy(() -> new CoreOptions(conf).formatTableCommitPublishThreadNum()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("format-table.commit.publish-thread-num") + .hasMessageContaining("1") + .hasMessageContaining("64"); + } + } } diff --git a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java index 77ac0ff03d3a..e892d010dafa 100644 --- a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java @@ -65,6 +65,7 @@ import static org.apache.paimon.shade.guava30.com.google.common.base.Throwables.getRootCause; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.Assertions.catchThrowable; import static org.assertj.core.api.Assertions.entry; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyBoolean; @@ -158,6 +159,52 @@ void testFileCommitFailureStillDiscardsUncommittedFiles() throws Exception { .createPartitions(anyList(), eq(true), any(), anyBoolean()); } + @Test + void testAbortAttemptsEveryRollbackAndReportsDeleteFailure() throws Exception { + Path tablePath = new Path(tempDir.toUri()); + Path refusedPath = new Path(tablePath, "part=p/data-refused.csv"); + Path removablePath = new Path(tablePath, "part=p/data-removable.csv"); + SelectiveRefusingDeleteFileIO fileIO = new SelectiveRefusingDeleteFileIO(refusedPath); + fileIO.writeFile(refusedPath, "published", false); + fileIO.writeFile(removablePath, "published", false); + + TwoPhaseOutputStream.Committer first = mock(TwoPhaseOutputStream.Committer.class); + when(first.targetPath()).thenReturn(refusedPath); + doThrow(new IOException("discard failed")).when(first).discard(fileIO); + TwoPhaseOutputStream.Committer second = mock(TwoPhaseOutputStream.Committer.class); + when(second.targetPath()).thenReturn(removablePath); + List messages = + Arrays.asList(new TwoPhaseCommitMessage(first), new TwoPhaseCommitMessage(second)); + FormatTableCommit commit = + new FormatTableCommit( + tablePath.toString(), + Collections.singletonList("part"), + fileIO, + false, + PARTITION_DEFAULT_NAME.defaultValue(), + false, + Identifier.create("rollback_db", "rollback_table"), + null, + null, + null, + null, + /* dynamicPartitionOverwrite */ true, + /* cleanupThreadNum */ 1); + + Throwable failure = catchThrowable(() -> commit.abort(messages)); + + assertThat(failure).isInstanceOf(RuntimeException.class); + assertThat(failureTree(failure)) + .extracting(Throwable::getMessage) + .contains( + "discard failed", + "Failed to delete published Format Table file " + refusedPath); + verify(first).discard(fileIO); + verify(second).discard(fileIO); + assertThat(fileIO.exists(refusedPath)).isTrue(); + assertThat(fileIO.exists(removablePath)).isFalse(); + } + @Test void testRegistersRawPartitionValuesForEscapedPath() throws Exception { Path tablePath = new Path(tempDir.toUri()); @@ -710,6 +757,243 @@ void testCatalogManagedBuilderUses64WayCleanupByDefault() throws Exception { } } + @Test + void testCatalogManagedBuilderPublishesSamePartitionConcurrentlyAndWaitsForBarrier() + throws Exception { + LocalFileIO fileIO = LocalFileIO.create(); + Path tablePath = new Path(tempDir.toUri()); + Path partitionPath = new Path(tablePath, "part=p"); + FormatTablePartitionManager partitionManager = mock(FormatTablePartitionManager.class); + CountDownLatch firstTwoStarted = new CountDownLatch(2); + CountDownLatch releaseFirst = new CountDownLatch(1); + CountDownLatch releaseSecond = new CountDownLatch(1); + CountDownLatch thirdFinished = new CountDownLatch(1); + AtomicInteger activePublishes = new AtomicInteger(); + AtomicInteger maxConcurrentPublishes = new AtomicInteger(); + List committers = new ArrayList<>(); + List messages = new ArrayList<>(); + for (int i = 0; i < 3; i++) { + int index = i; + TwoPhaseOutputStream.Committer committer = mock(TwoPhaseOutputStream.Committer.class); + when(committer.targetPath()).thenReturn(new Path(partitionPath, "data-" + i + ".csv")); + doAnswer( + invocation -> { + int active = activePublishes.incrementAndGet(); + maxConcurrentPublishes.updateAndGet( + previous -> Math.max(previous, active)); + try { + if (index == 0) { + firstTwoStarted.countDown(); + if (!releaseFirst.await(10, TimeUnit.SECONDS)) { + throw new IOException( + "Timed out waiting to release first publication"); + } + } else if (index == 1) { + firstTwoStarted.countDown(); + if (!releaseSecond.await(10, TimeUnit.SECONDS)) { + throw new IOException( + "Timed out waiting to release second publication"); + } + } + return null; + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IOException("File publication was interrupted", e); + } finally { + activePublishes.decrementAndGet(); + if (index == 2) { + thirdFinished.countDown(); + } + } + }) + .when(committer) + .commit(fileIO); + committers.add(committer); + messages.add(new TwoPhaseCommitMessage(committer)); + } + FormatTableCommit commit = + (FormatTableCommit) + formatTable( + tablePath, + fileIO, + partitionManager, + Collections.singletonMap( + CoreOptions.FORMAT_TABLE_COMMIT_PUBLISH_THREAD_NUM + .key(), + "2")) + .newBatchWriteBuilder() + .newCommit(); + + ExecutorService executor = Executors.newSingleThreadExecutor(); + Future result = executor.submit(() -> commit.commit(messages)); + try { + assertThat(firstTwoStarted.await(3, TimeUnit.SECONDS)).isTrue(); + assertThat(activePublishes).hasValue(2); + + releaseFirst.countDown(); + assertThat(thirdFinished.await(3, TimeUnit.SECONDS)).isTrue(); + assertThat(activePublishes).hasValue(1); + verify(partitionManager, never()) + .createPartitions(anyList(), eq(true), any(), anyBoolean()); + for (TwoPhaseOutputStream.Committer committer : committers) { + verify(committer, never()).clean(fileIO); + } + + releaseSecond.countDown(); + result.get(10, TimeUnit.SECONDS); + + assertThat(maxConcurrentPublishes).hasValue(2); + for (TwoPhaseOutputStream.Committer committer : committers) { + verify(committer).clean(fileIO); + } + verify(partitionManager).createPartitions(anyList(), eq(true), any(), eq(false)); + } finally { + releaseFirst.countDown(); + releaseSecond.countDown(); + if (!result.isDone()) { + result.get(10, TimeUnit.SECONDS); + } + executor.shutdownNow(); + } + } + + @Test + void testPublishFailureDrainsRunningWorkBeforeAbort() throws Exception { + LocalFileIO fileIO = LocalFileIO.create(); + Path tablePath = new Path(tempDir.toUri()); + Path partitionPath = new Path(tablePath, "part=p"); + FormatTablePartitionManager partitionManager = mock(FormatTablePartitionManager.class); + CountDownLatch secondStarted = new CountDownLatch(1); + CountDownLatch releaseSecond = new CountDownLatch(1); + AtomicInteger activePublishes = new AtomicInteger(); + ConcurrentLinkedQueue activePublishesAtDiscard = new ConcurrentLinkedQueue<>(); + List committers = new ArrayList<>(); + List targetPaths = new ArrayList<>(); + List messages = new ArrayList<>(); + for (int i = 0; i < 3; i++) { + TwoPhaseOutputStream.Committer committer = mock(TwoPhaseOutputStream.Committer.class); + Path targetPath = new Path(partitionPath, "data-" + i + ".csv"); + when(committer.targetPath()).thenReturn(targetPath); + doAnswer( + invocation -> { + activePublishesAtDiscard.add(activePublishes.get()); + return null; + }) + .when(committer) + .discard(fileIO); + committers.add(committer); + targetPaths.add(targetPath); + messages.add(new TwoPhaseCommitMessage(committer)); + } + doAnswer( + invocation -> { + activePublishes.incrementAndGet(); + try { + if (!secondStarted.await(10, TimeUnit.SECONDS)) { + throw new IOException("The second publication did not start"); + } + fileIO.writeFile(targetPaths.get(0), "published", false); + throw new IOException("publish failed"); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IOException("File publication was interrupted", e); + } finally { + activePublishes.decrementAndGet(); + } + }) + .when(committers.get(0)) + .commit(fileIO); + doAnswer( + invocation -> { + activePublishes.incrementAndGet(); + secondStarted.countDown(); + try { + if (!releaseSecond.await(10, TimeUnit.SECONDS)) { + throw new IOException( + "Timed out waiting to release publication"); + } + fileIO.writeFile(targetPaths.get(1), "published", false); + return null; + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IOException("File publication was interrupted", e); + } finally { + activePublishes.decrementAndGet(); + } + }) + .when(committers.get(1)) + .commit(fileIO); + FormatTableCommit commit = + (FormatTableCommit) + formatTable( + tablePath, + fileIO, + partitionManager, + Collections.singletonMap( + CoreOptions.FORMAT_TABLE_COMMIT_PUBLISH_THREAD_NUM + .key(), + "2")) + .newBatchWriteBuilder() + .newCommit(); + + ExecutorService executor = Executors.newSingleThreadExecutor(); + Future result = executor.submit(() -> commit.commit(messages)); + try { + assertThat(secondStarted.await(10, TimeUnit.SECONDS)).isTrue(); + assertThatThrownBy(() -> result.get(300, TimeUnit.MILLISECONDS)) + .isInstanceOf(TimeoutException.class); + for (TwoPhaseOutputStream.Committer committer : committers) { + verify(committer, never()).discard(fileIO); + } + + releaseSecond.countDown(); + assertThat(getRootCause(awaitFailure(result))).hasMessage("publish failed"); + + verify(committers.get(2), never()).commit(fileIO); + assertThat(activePublishesAtDiscard).containsExactly(0, 0, 0); + for (TwoPhaseOutputStream.Committer committer : committers) { + verify(committer).discard(fileIO); + } + assertThat(fileIO.exists(targetPaths.get(0))).isFalse(); + assertThat(fileIO.exists(targetPaths.get(1))).isFalse(); + } finally { + releaseSecond.countDown(); + if (!result.isDone()) { + try { + result.get(10, TimeUnit.SECONDS); + } catch (ExecutionException ignored) { + // The test expects the first publication to fail. + } + } + executor.shutdownNow(); + } + } + + @Test + void testPublishConcurrencyIsGatedToCatalogManagedPartitionedTables() throws Exception { + LocalFileIO fileIO = LocalFileIO.create(); + Path tablePath = new Path(tempDir.toUri()); + Map options = + Collections.singletonMap( + CoreOptions.FORMAT_TABLE_COMMIT_PUBLISH_THREAD_NUM.key(), "64"); + + FormatTableCommit filesystemDiscovered = + (FormatTableCommit) + formatTable(new Path(tablePath, "filesystem"), fileIO, null, options) + .newBatchWriteBuilder() + .newCommit(); + assertPublishesOnCaller( + filesystemDiscovered, fileIO, new Path(tablePath, "filesystem/part=p")); + + FormatTableCommit unpartitioned = + builderUnpartitionedOverwriteCommit( + new Path(tablePath, "unpartitioned"), + fileIO, + mock(FormatTablePartitionManager.class), + options); + assertPublishesOnCaller(unpartitioned, fileIO, new Path(tablePath, "unpartitioned")); + } + @Test void testCatalogManagedBuilderHonorsConfiguredSerialCleanup() throws Exception { SerialProbeFileIO fileIO = new SerialProbeFileIO(); @@ -885,7 +1169,7 @@ void testCleanupFailureStopsNewSubmissionsAndDrainsTheAlreadyRunningDelete() thr assertThat(getRootCause(awaitFailure(result))) .hasMessage("delete failed at input position 0"); assertThat(fileIO.attemptedFiles()) - .containsExactlyInAnyOrder("data-000.csv", "data-001.csv"); + .containsExactlyInAnyOrder("data-000.csv", "data-001.csv", "data-new.csv"); assertThat(fileIO.successfulFiles()).containsExactly("data-001.csv"); verify(committer, never()).commit(fileIO); } finally { @@ -1485,10 +1769,33 @@ private static void writeOldFiles(LocalFileIO fileIO, Path partitionPath, int co } } + private static void assertPublishesOnCaller( + FormatTableCommit commit, FileIO fileIO, Path parent) throws IOException { + Thread caller = Thread.currentThread(); + ConcurrentLinkedQueue publishThreads = new ConcurrentLinkedQueue<>(); + List messages = new ArrayList<>(); + for (int i = 0; i < 2; i++) { + TwoPhaseOutputStream.Committer committer = mock(TwoPhaseOutputStream.Committer.class); + when(committer.targetPath()).thenReturn(new Path(parent, "data-" + i + ".csv")); + doAnswer( + invocation -> { + publishThreads.add(Thread.currentThread()); + return null; + }) + .when(committer) + .commit(fileIO); + messages.add(new TwoPhaseCommitMessage(committer)); + } + + commit.commit(messages); + + assertThat(publishThreads).containsExactly(caller, caller); + } + private static ExecutionException awaitFailure(Future future) throws Exception { try { future.get(10, TimeUnit.SECONDS); - throw new AssertionError("Expected cleanup commit to fail"); + throw new AssertionError("Expected Format Table commit to fail"); } catch (ExecutionException expected) { return expected; } @@ -1511,6 +1818,22 @@ private static void collectFailures(Throwable throwable, List failure collectFailures(throwable.getCause(), failures); } + private static class SelectiveRefusingDeleteFileIO extends LocalFileIO { + + private static final long serialVersionUID = 1L; + + private final Path refusedPath; + + private SelectiveRefusingDeleteFileIO(Path refusedPath) { + this.refusedPath = refusedPath; + } + + @Override + public boolean delete(Path path, boolean recursive) throws IOException { + return path.equals(refusedPath) ? false : super.delete(path, recursive); + } + } + private static class ParallelDeleteFileIO extends LocalFileIO { private final int firstWaveSize; From 206054642148ddb9708ad37e1b7a6716bf2599b4 Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Thu, 27 Aug 2026 09:26:02 +0800 Subject: [PATCH 03/10] [api] Order the interrupt hook against the assertion in ThreadPoolUtilsTest testCloseCancelsQueuedTasksAndWaitsUninterruptibly reads runQueuedTaskOnInterrupt as soon as workerInterrupted is released. The hook that clears the flag does so after super.interrupt(), and it is super.interrupt() that wakes the worker and releases that latch. The read and the clear are therefore unordered: let the closing thread lose the CPU between the two and the assertion sees a flag the hook has not consumed yet. That is a fixture defect, not a product one. close still cancels every unstarted task before it interrupts a running one, and the invariants the test exists for, executions == 2 and thirdExecuted == false, are asserted separately. Have the hook count down a latch once it is done, and await that latch before reading the flag. The test still kills the bug it was written for: reversing the cancel and interrupt loops in close fails it on executions. It reproduced on every run pinned to a single CPU and on none of the unpinned ones, which is why a low core count runner saw it and a development machine did not. --- .../test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java b/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java index b61593482f0e..5c3fc1e4e942 100644 --- a/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java +++ b/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java @@ -322,6 +322,7 @@ public void testWorkerRunsWithTheSubmittingSubject() throws Exception { public void testCloseCancelsQueuedTasksAndWaitsUninterruptibly() throws Exception { LinkedBlockingQueue taskQueue = new LinkedBlockingQueue<>(); AtomicBoolean runQueuedTaskOnInterrupt = new AtomicBoolean(); + CountDownLatch interruptHookFinished = new CountDownLatch(1); // If close interrupts the worker before cancelling queued tasks, interrupt() runs the // queued task and exposes the ordering bug. ThreadPoolExecutor workers = @@ -341,6 +342,7 @@ public void interrupt() { if (queuedTask != null) { queuedTask.run(); } + interruptHookFinished.countDown(); } } }); @@ -387,6 +389,7 @@ public void interrupt() { }); assertThat(closeStarted.await(3, TimeUnit.SECONDS)).isTrue(); assertThat(workerInterrupted.await(3, TimeUnit.SECONDS)).isTrue(); + assertThat(interruptHookFinished.await(3, TimeUnit.SECONDS)).isTrue(); assertThat(runQueuedTaskOnInterrupt).isFalse(); assertThat(closeResult.isDone()).isFalse(); From c7922b138512e9172a6c16020ac7a6ef20a0567e Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Thu, 27 Aug 2026 10:26:22 +0800 Subject: [PATCH 04/10] [api][core] Fix Format Table rollback and scheduling edge cases --- .../apache/paimon/utils/ThreadPoolUtils.java | 73 ++++-- .../paimon/utils/ThreadPoolUtilsTest.java | 157 ++++++++++--- .../table/format/FormatTableCommit.java | 60 ++++- .../table/format/TwoPhaseCommitMessage.java | 12 + .../paimon/utils/ManifestReadThreadPool.java | 2 +- .../org/apache/paimon/CoreOptionsTest.java | 30 +-- .../FormatTableCommitCompatibilityTest.java | 56 +++++ .../FormatTableCommitStatisticsTest.java | 18 +- .../table/format/FormatTableCommitTest.java | 218 +++++++++++++++++- 9 files changed, 522 insertions(+), 104 deletions(-) create mode 100644 paimon-core/src/test/java/org/apache/paimon/table/FormatTableCommitCompatibilityTest.java diff --git a/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java b/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java index 37db694e93c3..3e5687aef254 100644 --- a/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java +++ b/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java @@ -50,7 +50,7 @@ /** Utils for thread pool. */ public class ThreadPoolUtils { - /** An iterator which waits for its active batch to quiesce when closed. */ + /** An iterator which waits for its active tasks to quiesce when closed. */ public interface CloseableBatchIterator extends Iterator, AutoCloseable { @Override @@ -137,32 +137,43 @@ private void advanceIfNeeded() { } /** - * Processes a bounded number of inputs in parallel and returns results in input order. + * Processes one bounded batch at a time and returns results in input order. * - *

The caller must close the iterator to cancel unstarted tasks and wait for running tasks. + *

Closing cancels unstarted tasks, interrupts running tasks, and waits for every submitted + * task to finish. */ public static CloseableBatchIterator sequentialBatchedExecuteCloseable( ExecutorService executor, Function> processor, List input, int queueSize) { - return newSequentialBatchIterator(executor, processor, input.iterator(), queueSize, true); + return newSequentialBatchIterator( + executor, + processor, + input.iterator(), + queueSize, + SchedulingMode.BATCHED_CANCEL_RUNNING); } /** - * As {@link #sequentialBatchedExecuteCloseable}, but closing waits for a task that has already - * started instead of interrupting it. + * Processes a bounded sliding window of inputs from the iterator and returns results in input + * order. + * + *

Unlike {@link #sequentialBatchedExecuteCloseable}, closing waits for a task that has + * already started instead of interrupting it. * *

Use this when a task changes stored state. Interrupting a delete or a write halfway leaves * the caller unable to say whether it took effect, so a caller that has to know the outcome of * everything it handed out cannot let close cancel work that is already running. */ - public static CloseableBatchIterator sequentialBatchedExecuteAwaitRunningTasksOnClose( - ExecutorService executor, - Function> processor, - Iterator input, - int queueSize) { - return newSequentialBatchIterator(executor, processor, input, queueSize, false); + public static + CloseableBatchIterator sequentialSlidingWindowExecuteAwaitRunningTasksOnClose( + ExecutorService executor, + Function> processor, + Iterator input, + int queueSize) { + return newSequentialBatchIterator( + executor, processor, input, queueSize, SchedulingMode.SLIDING_AWAIT_RUNNING); } private static CloseableBatchIterator newSequentialBatchIterator( @@ -170,12 +181,11 @@ private static CloseableBatchIterator newSequentialBatchIterator( Function> processor, Iterator input, int queueSize, - boolean cancelRunningOnClose) { + SchedulingMode mode) { if (queueSize <= 0) { throw new NegativeArraySizeException("queue size should not be negative"); } - return new SequentialBatchIterator<>( - executor, processor, input, queueSize, cancelRunningOnClose); + return new SequentialBatchIterator<>(executor, processor, input, queueSize, mode); } public static void randomlyOnlyExecute( @@ -249,13 +259,26 @@ public static void awaitAllFutures(List> futures) { } } + private enum SchedulingMode { + BATCHED_CANCEL_RUNNING(true, false), + SLIDING_AWAIT_RUNNING(false, true); + + private final boolean cancelRunningOnClose; + private final boolean slidingWindow; + + SchedulingMode(boolean cancelRunningOnClose, boolean slidingWindow) { + this.cancelRunningOnClose = cancelRunningOnClose; + this.slidingWindow = slidingWindow; + } + } + private static class SequentialBatchIterator implements CloseableBatchIterator { private final ExecutorService executor; private final Function> processor; private final Iterator input; private final int queueSize; - private final boolean cancelRunningOnClose; + private final SchedulingMode mode; private final Queue> activeTasks = new ArrayDeque<>(); private final Object submissionLock = new Object(); @@ -269,12 +292,12 @@ private SequentialBatchIterator( Function> processor, Iterator input, int queueSize, - boolean cancelRunningOnClose) { + SchedulingMode mode) { this.executor = executor; this.processor = processor; this.input = input; this.queueSize = queueSize; - this.cancelRunningOnClose = cancelRunningOnClose; + this.mode = mode; } @Override @@ -301,7 +324,9 @@ private void advanceIfNeeded() { next = activeResults.next(); continue; } - fillWindow(); + if (mode.slidingWindow || activeTasks.isEmpty()) { + fillWindow(); + } if (activeTasks.isEmpty()) { return; } @@ -363,7 +388,7 @@ public synchronized void close() { failure = firstOrSuppressed(cleanupFailure, failure); } } - if (cancelRunningOnClose) { + if (mode.cancelRunningOnClose) { for (BatchTask task : activeTasks) { try { task.interruptIfRunning(); @@ -458,16 +483,16 @@ public void run() { stopSubmission.run(); } finally { currentThread.setContextClassLoader(originalClassLoader); + synchronized (this) { + runner = null; + state = FINISHED; + } // Reset the flag to its entry state so a cancelled task cannot leak an interrupt // to a reused worker and a direct executor cannot clear its caller's interrupt. Thread.interrupted(); if (interruptedOnEntry) { currentThread.interrupt(); } - synchronized (this) { - runner = null; - state = FINISHED; - } completion.countDown(); } } diff --git a/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java b/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java index 5c3fc1e4e942..30708c82d984 100644 --- a/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java +++ b/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java @@ -29,6 +29,7 @@ import java.security.AccessController; import java.security.PrivilegedAction; +import java.util.AbstractList; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -59,6 +60,20 @@ public void testCloseableBatchReturnsInOrderAndBoundsSubmission() throws Excepti CountDownLatch secondFinished = new CountDownLatch(1); CountDownLatch thirdStarted = new CountDownLatch(1); CountDownLatch releaseFirst = new CountDownLatch(1); + AtomicInteger inputsRead = new AtomicInteger(); + List inputs = + new AbstractList() { + @Override + public Integer get(int index) { + inputsRead.incrementAndGet(); + return index; + } + + @Override + public int size() { + return 4; + } + }; CloseableBatchIterator iterator = ThreadPoolUtils.sequentialBatchedExecuteCloseable( workers, @@ -73,7 +88,7 @@ public void testCloseableBatchReturnsInOrderAndBoundsSubmission() throws Excepti } return Collections.singletonList(input); }, - Arrays.asList(0, 1, 2, 3), + inputs, 2); try { @@ -86,19 +101,22 @@ public void testCloseableBatchReturnsInOrderAndBoundsSubmission() throws Excepti assertThat(firstStarted.await(3, TimeUnit.SECONDS)).isTrue(); assertThat(secondFinished.await(3, TimeUnit.SECONDS)).isTrue(); - workers.submit(() -> {}).get(3, TimeUnit.SECONDS); + assertThat(inputsRead).hasValue(2); assertThat(thirdStarted.getCount()).isOne(); assertThat(firstResult.isDone()).isFalse(); releaseFirst.countDown(); List results = new ArrayList<>(); results.add(firstResult.get(3, TimeUnit.SECONDS)); - // Consuming input 0 frees one slot. The next lookup refills it before input 1 is - // consumed, instead of waiting for the whole window to drain. + // Reading the remaining result must not submit the next batch. assertThat(iterator.hasNext()).isTrue(); - assertThat(thirdStarted.await(3, TimeUnit.SECONDS)).isTrue(); + assertThat(inputsRead).hasValue(2); + assertThat(thirdStarted.getCount()).isOne(); results.add(iterator.next()); + + // The next lookup starts the second batch only after the first batch is drained. assertThat(iterator.hasNext()).isTrue(); + assertThat(thirdStarted.await(3, TimeUnit.SECONDS)).isTrue(); results.add(iterator.next()); assertThat(iterator.hasNext()).isTrue(); results.add(iterator.next()); @@ -135,7 +153,7 @@ public Integer next() { } }; try (CloseableBatchIterator iterator = - ThreadPoolUtils.sequentialBatchedExecuteAwaitRunningTasksOnClose( + ThreadPoolUtils.sequentialSlidingWindowExecuteAwaitRunningTasksOnClose( workers, value -> { if (value == 0) { @@ -148,6 +166,8 @@ public Integer next() { releaseFirst.countDown(); assertThat(iterator.next()).isEqualTo(0); assertThat(inputsRead).hasValue(4); + assertThat(iterator.next()).isEqualTo(1); + assertThat(inputsRead).hasValue(5); } finally { workers.shutdownNow(); } @@ -169,7 +189,7 @@ public void testWorkerFailureStopsNewSubmissions() throws Exception { return value; }); CloseableBatchIterator iterator = - ThreadPoolUtils.sequentialBatchedExecuteAwaitRunningTasksOnClose( + ThreadPoolUtils.sequentialSlidingWindowExecuteAwaitRunningTasksOnClose( workers, value -> { if (value == 0) { @@ -279,34 +299,25 @@ public void testWorkerRunsWithTheSubmittingSubject() throws Exception { Subject secondSubject = new Subject(); List seenSubjects = new ArrayList<>(); List seenWorkers = new ArrayList<>(); + PrivilegedAction readSubjectFromWorker = + () -> { + try (CloseableBatchIterator iterator = + ThreadPoolUtils.sequentialBatchedExecuteCloseable( + workers, + ignored -> { + seenWorkers.add(Thread.currentThread()); + return Collections.singletonList( + Subject.getSubject(AccessController.getContext())); + }, + Collections.singletonList(0), + 1)) { + return iterator.next(); + } + }; try { for (Subject subject : Arrays.asList(firstSubject, secondSubject)) { - seenSubjects.add( - Subject.doAs( - subject, - (PrivilegedAction) - () -> { - try (CloseableBatchIterator iterator = - ThreadPoolUtils - .sequentialBatchedExecuteCloseable( - workers, - ignored -> { - seenWorkers.add( - Thread - .currentThread()); - return Collections - .singletonList( - Subject - .getSubject( - AccessController - .getContext())); - }, - Collections.singletonList(0), - 1)) { - return iterator.next(); - } - })); + seenSubjects.add(Subject.doAs(subject, readSubjectFromWorker)); } assertThat(seenWorkers.get(1)).isSameAs(seenWorkers.get(0)); @@ -414,6 +425,88 @@ public void interrupt() { } } + @Test + public void testCloseDoesNotLeakAnInterruptAfterTaskCompletion() throws Exception { + AtomicBoolean delayInterruptUntilWorkerFinishes = new AtomicBoolean(); + AtomicBoolean workerBlockedBeforeInterrupt = new AtomicBoolean(); + AtomicBoolean interruptedAfterTask = new AtomicBoolean(); + AtomicInteger completedTasks = new AtomicInteger(); + CountDownLatch secondStarted = new CountDownLatch(1); + CountDownLatch releaseSecond = new CountDownLatch(1); + CountDownLatch secondFinished = new CountDownLatch(1); + ThreadPoolExecutor workers = + new ThreadPoolExecutor( + 1, + 1, + 0L, + TimeUnit.MILLISECONDS, + new LinkedBlockingQueue<>(), + runnable -> + new Thread(runnable) { + // close holds BatchTask's monitor here. Release the processor, + // wait for the worker to block publishing FINISHED, then + // deliver the interrupt in the old leak window. + @Override + public void interrupt() { + if (delayInterruptUntilWorkerFinishes.compareAndSet( + true, false)) { + releaseSecond.countDown(); + long deadline = + System.nanoTime() + TimeUnit.SECONDS.toNanos(3); + while (getState() != State.BLOCKED + && System.nanoTime() < deadline) { + Thread.yield(); + } + workerBlockedBeforeInterrupt.set( + getState() == State.BLOCKED); + } + super.interrupt(); + } + }) { + @Override + protected void afterExecute(Runnable runnable, Throwable throwable) { + super.afterExecute(runnable, throwable); + if (completedTasks.incrementAndGet() == 2) { + interruptedAfterTask.set(Thread.currentThread().isInterrupted()); + secondFinished.countDown(); + } + } + }; + ExecutorService closer = Executors.newSingleThreadExecutor(); + CloseableBatchIterator iterator = + ThreadPoolUtils.sequentialBatchedExecuteCloseable( + workers, + input -> { + if (input == 1) { + secondStarted.countDown(); + await(releaseSecond); + } + return Collections.singletonList(input); + }, + Arrays.asList(0, 1), + 2); + + try { + assertThat(iterator.next()).isZero(); + assertThat(secondStarted.await(3, TimeUnit.SECONDS)).isTrue(); + delayInterruptUntilWorkerFinishes.set(true); + + Future closeResult = closer.submit(iterator::close); + assertThat(secondFinished.await(3, TimeUnit.SECONDS)).isTrue(); + closeResult.get(3, TimeUnit.SECONDS); + + assertThat(workerBlockedBeforeInterrupt).isTrue(); + assertThat(interruptedAfterTask).isFalse(); + } finally { + releaseSecond.countDown(); + iterator.close(); + closer.shutdownNow(); + workers.shutdownNow(); + assertThat(closer.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); + assertThat(workers.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); + } + } + @Test public void testClosePreservesPrimaryErrorAndSuppressesWorkerError() throws Exception { ThreadPoolExecutor workers = (ThreadPoolExecutor) Executors.newFixedThreadPool(2); diff --git a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java index f1442d85b0d6..c09cc89cd1b7 100644 --- a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java +++ b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java @@ -66,7 +66,7 @@ import static org.apache.paimon.table.format.FormatBatchWriteBuilder.validateStaticPartition; import static org.apache.paimon.utils.ExceptionUtils.firstOrSuppressed; import static org.apache.paimon.utils.ThreadPoolUtils.CloseableBatchIterator; -import static org.apache.paimon.utils.ThreadPoolUtils.sequentialBatchedExecuteAwaitRunningTasksOnClose; +import static org.apache.paimon.utils.ThreadPoolUtils.sequentialSlidingWindowExecuteAwaitRunningTasksOnClose; /** Commit for Format Table. */ public class FormatTableCommit implements BatchTableCommit { @@ -94,6 +94,35 @@ public class FormatTableCommit implements BatchTableCommit { private final int publishThreadNum; public FormatTableCommit( + String location, + List partitionKeys, + FileIO fileIO, + boolean formatTablePartitionOnlyValueInPath, + String defaultPartName, + boolean overwrite, + Identifier tableIdentifier, + @Nullable Map staticPartitions, + @Nullable String syncHiveUri, + CatalogContext catalogContext, + @Nullable FormatTablePartitionManager partitionManager, + boolean dynamicPartitionOverwrite) { + this( + location, + partitionKeys, + fileIO, + formatTablePartitionOnlyValueInPath, + defaultPartName, + overwrite, + tableIdentifier, + staticPartitions, + syncHiveUri, + catalogContext, + partitionManager, + dynamicPartitionOverwrite, + 1); + } + + FormatTableCommit( String location, List partitionKeys, FileIO fileIO, @@ -278,6 +307,7 @@ public void commit(List commitMessages) { } if (reportsStatistics) { reportPartitions( + messages, partitionSpecs, statisticsByPartition, clearedPartitionPaths, @@ -286,8 +316,10 @@ public void commit(List commitMessages) { } else if (partitionManager != null && !partitionSpecs.isEmpty()) { // Concurrent writers may touch the same partition, so registration ignores the // ones that already exist rather than failing the commit. + markPublishedTargetsToPreserveOnAbort(messages); partitionManager.createPartitions(new ArrayList<>(partitionSpecs), true); } + boolean hiveMutationStarted = false; for (Map partitionSpec : partitionSpecs) { if (hiveCatalog != null) { try { @@ -296,6 +328,10 @@ public void commit(List commitMessages) { } Method hiveCreatePartitionsInHmsMethod = getHiveCreatePartitionsInHmsMethod(); + if (!hiveMutationStarted) { + markPublishedTargetsToPreserveOnAbort(messages); + hiveMutationStarted = true; + } hiveCreatePartitionsInHmsMethod.invoke( hiveCatalog, tableIdentifier, @@ -339,7 +375,7 @@ private void publishMessages(List messages) throws IOExce } try (CloseableBatchIterator published = - sequentialBatchedExecuteAwaitRunningTasksOnClose( + sequentialSlidingWindowExecuteAwaitRunningTasksOnClose( COMMIT_EXECUTOR, this::publishMessage, messages.iterator(), @@ -361,6 +397,13 @@ private List publishMessage(TwoPhaseCommitMessage message) { } } + private static void markPublishedTargetsToPreserveOnAbort( + List messages) { + for (TwoPhaseCommitMessage message : messages) { + message.markPublishedTargetToPreserveOnAbort(); + } + } + /** * Registers the partitions this commit touched, carrying the statistics of what it wrote. An * overwrite also empties partitions it writes nothing to - those below a static prefix, and @@ -370,6 +413,7 @@ private List publishMessage(TwoPhaseCommitMessage message) { * reports every partition it emptied. */ private void reportPartitions( + List messages, Set> writtenPartitionSpecs, Map, PartitionStatistics> statisticsByPartition, Set clearedPartitionPaths, @@ -391,6 +435,7 @@ private void reportPartitions( } // A commit that replaced what the partitions held reports a total; an appending one saw // only its own files, so its numbers are an increment. + markPublishedTargetsToPreserveOnAbort(messages); partitionManager.createPartitions( new ArrayList<>(specs), true, @@ -549,8 +594,12 @@ public void abort(List commitMessages) { continue; } - TwoPhaseOutputStream.Committer committer = - ((TwoPhaseCommitMessage) commitMessage).getCommitter(); + TwoPhaseCommitMessage twoPhaseCommitMessage = (TwoPhaseCommitMessage) commitMessage; + if (twoPhaseCommitMessage.shouldPreservePublishedTargetOnAbort()) { + continue; + } + + TwoPhaseOutputStream.Committer committer = twoPhaseCommitMessage.getCommitter(); try { committer.discard(fileIO); } catch (Throwable discardFailure) { @@ -689,7 +738,7 @@ private Set deletePreviousDataFiles( // the iterator is what stops new deletes and waits for the ones already handed out, // so a failure cannot leave a worker still deleting after this method returns. try (CloseableBatchIterator cleared = - sequentialBatchedExecuteAwaitRunningTasksOnClose( + sequentialSlidingWindowExecuteAwaitRunningTasksOnClose( COMMIT_EXECUTOR, this::deleteAndReportCleared, dataFiles, threadNum)) { while (cleared.hasNext()) { clearedPartitionPaths.add(cleared.next()); @@ -889,6 +938,7 @@ private void truncate(List> partitionSpecs) { // too, so the catalog stops describing files that are gone. try { reportPartitions( + Collections.emptyList(), Collections.emptySet(), emptied, clearedPartitionPaths, diff --git a/paimon-core/src/main/java/org/apache/paimon/table/format/TwoPhaseCommitMessage.java b/paimon-core/src/main/java/org/apache/paimon/table/format/TwoPhaseCommitMessage.java index f44c5e9b8904..a8d044553abd 100644 --- a/paimon-core/src/main/java/org/apache/paimon/table/format/TwoPhaseCommitMessage.java +++ b/paimon-core/src/main/java/org/apache/paimon/table/format/TwoPhaseCommitMessage.java @@ -40,6 +40,10 @@ public class TwoPhaseCommitMessage implements CommitMessage { private final long recordCount; private final long fileSizeInBytes; + // Set before external partition metadata may become durable. Keeping this state in the + // serialized message prevents a later abort instance from deleting a referenced target. + private boolean preservePublishedTargetOnAbort; + public TwoPhaseCommitMessage(TwoPhaseOutputStream.Committer committer) { this(committer, PartitionStatistics.UNKNOWN, PartitionStatistics.UNKNOWN); } @@ -70,6 +74,14 @@ public TwoPhaseOutputStream.Committer getCommitter() { return committer; } + void markPublishedTargetToPreserveOnAbort() { + preservePublishedTargetOnAbort = true; + } + + boolean shouldPreservePublishedTargetOnAbort() { + return preservePublishedTargetOnAbort; + } + /** Rows in this file, or {@link PartitionStatistics#UNKNOWN} when nobody counted them. */ public long recordCount() { return recordCount; diff --git a/paimon-core/src/main/java/org/apache/paimon/utils/ManifestReadThreadPool.java b/paimon-core/src/main/java/org/apache/paimon/utils/ManifestReadThreadPool.java index 1e106cb3d4e1..7d92195ae8ca 100644 --- a/paimon-core/src/main/java/org/apache/paimon/utils/ManifestReadThreadPool.java +++ b/paimon-core/src/main/java/org/apache/paimon/utils/ManifestReadThreadPool.java @@ -61,7 +61,7 @@ public static Iterable sequentialBatchedExecute( executor, processor, input, effectiveThreadNum(threadNum, executor)); } - /** Processes a bounded number of inputs in parallel and waits for submitted tasks on close. */ + /** Processes one bounded batch in parallel and waits for it when closed. */ public static ThreadPoolUtils.CloseableBatchIterator sequentialBatchedExecuteCloseable( Function> processor, List input, @Nullable Integer threadNum) { diff --git a/paimon-core/src/test/java/org/apache/paimon/CoreOptionsTest.java b/paimon-core/src/test/java/org/apache/paimon/CoreOptionsTest.java index ea7513208702..93bde01a2f1b 100644 --- a/paimon-core/src/test/java/org/apache/paimon/CoreOptionsTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/CoreOptionsTest.java @@ -303,27 +303,15 @@ public void testFormatTableCommitCleanupThreadNumDefaultsTo64AndAcceptsBounds() @Test public void testFormatTableCommitCleanupThreadNumRejectsValuesOutsideSupportedRange() { - Options conf = new Options(); - conf.set(CoreOptions.FORMAT_TABLE_COMMIT_CLEANUP_THREAD_NUM, 0); - assertThatThrownBy(() -> new CoreOptions(conf).formatTableCommitCleanupThreadNum()) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("format-table.commit.cleanup-thread-num") - .hasMessageContaining("1") - .hasMessageContaining("64"); - - conf.set(CoreOptions.FORMAT_TABLE_COMMIT_CLEANUP_THREAD_NUM, -1); - assertThatThrownBy(() -> new CoreOptions(conf).formatTableCommitCleanupThreadNum()) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("format-table.commit.cleanup-thread-num") - .hasMessageContaining("1") - .hasMessageContaining("64"); - - conf.set(CoreOptions.FORMAT_TABLE_COMMIT_CLEANUP_THREAD_NUM, 65); - assertThatThrownBy(() -> new CoreOptions(conf).formatTableCommitCleanupThreadNum()) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("format-table.commit.cleanup-thread-num") - .hasMessageContaining("1") - .hasMessageContaining("64"); + for (int invalid : new int[] {0, -1, 65}) { + Options conf = new Options(); + conf.set(CoreOptions.FORMAT_TABLE_COMMIT_CLEANUP_THREAD_NUM, invalid); + assertThatThrownBy(() -> new CoreOptions(conf).formatTableCommitCleanupThreadNum()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("format-table.commit.cleanup-thread-num") + .hasMessageContaining("1") + .hasMessageContaining("64"); + } } @Test diff --git a/paimon-core/src/test/java/org/apache/paimon/table/FormatTableCommitCompatibilityTest.java b/paimon-core/src/test/java/org/apache/paimon/table/FormatTableCommitCompatibilityTest.java new file mode 100644 index 000000000000..632ea63c46d2 --- /dev/null +++ b/paimon-core/src/test/java/org/apache/paimon/table/FormatTableCommitCompatibilityTest.java @@ -0,0 +1,56 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.paimon.table; + +import org.apache.paimon.catalog.Identifier; +import org.apache.paimon.fs.Path; +import org.apache.paimon.fs.local.LocalFileIO; +import org.apache.paimon.table.format.FormatTableCommit; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.util.Collections; + +/** Compatibility tests for the public Format Table commit API. */ +class FormatTableCommitCompatibilityTest { + + @TempDir java.nio.file.Path tempDir; + + @Test + void testLegacyPublicConstructorCanCommitOutsideFormatPackage() throws Exception { + Path tablePath = new Path(tempDir.toUri()); + try (FormatTableCommit commit = + new FormatTableCommit( + tablePath.toString(), + Collections.emptyList(), + LocalFileIO.create(), + false, + "__DEFAULT_PARTITION__", + false, + Identifier.create("compatibility_db", "compatibility_table"), + null, + null, + null, + null, + true)) { + commit.commit(Collections.emptyList()); + } + } +} diff --git a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitStatisticsTest.java b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitStatisticsTest.java index 1a52eaab887d..ac857e674030 100644 --- a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitStatisticsTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitStatisticsTest.java @@ -972,7 +972,7 @@ void testTheNumbersReachTheCatalogThroughTheWriteBuilder() throws Exception { } @Test - void testAFailedReportFailsTheCommitAndDiscardsWhatItWrote() throws Exception { + void testAFailedReportPreservesPublishedTargetWhenCatalogOutcomeIsUnknown() throws Exception { LocalFileIO fileIO = LocalFileIO.create(); Path tablePath = new Path(tempDir.toUri()); FormatTablePartitionManager partitionManager = mock(FormatTablePartitionManager.class); @@ -983,20 +983,19 @@ void testAFailedReportFailsTheCommitAndDiscardsWhatItWrote() throws Exception { CommitMessage message = writtenFile(fileIO, tablePath, "year=2025/month=10", 3, 128); Path written = ((TwoPhaseCommitMessage) message).getCommitter().targetPath(); - // Registration and statistics ride in one request, so a failure says nothing about - // whether the partition was registered. Committing anyway would leave data behind that - // nothing points at. + // Once the catalog call starts, an exception cannot prove whether registration took + // effect. Removing the target could leave durable metadata pointing at a missing file. assertThatThrownBy( () -> commit(tablePath, fileIO, partitionManager, false, null) .commit(Collections.singletonList(message))) .hasRootCause(failure); - assertThat(fileIO.exists(written)).isFalse(); + assertThat(fileIO.exists(written)).isTrue(); } @Test - void testAFailedReportOfAnOverwriteLeavesThePartitionEmpty() throws Exception { + void testFailedOverwriteReportPreservesReplacementAfterDeletingOldData() throws Exception { LocalFileIO fileIO = LocalFileIO.create(); Path tablePath = new Path(tempDir.toUri()); FormatTablePartitionManager partitionManager = mock(FormatTablePartitionManager.class); @@ -1018,10 +1017,9 @@ void testAFailedReportOfAnOverwriteLeavesThePartitionEmpty() throws Exception { .commit(Collections.singletonList(message))) .hasRootCauseMessage("catalog says 429"); - // The state this leaves is worth stating rather than discovering: the overwrite already - // deleted what the partition held, and the abort takes back what it wrote, so the - // partition is empty on disk while the catalog still describes what used to be there. - assertThat(fileIO.exists(written)).isFalse(); + // The old file cannot be restored. Keep the replacement because the failed catalog call + // may already have made its metadata durable. + assertThat(fileIO.exists(written)).isTrue(); assertThat(fileIO.exists(new Path(tablePath, "year=2025/month=10/old-data.csv"))).isFalse(); } diff --git a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java index e892d010dafa..f5dc81c1f5e5 100644 --- a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java @@ -19,6 +19,8 @@ package org.apache.paimon.table.format; import org.apache.paimon.CoreOptions; +import org.apache.paimon.catalog.Catalog; +import org.apache.paimon.catalog.FileSystemCatalog; import org.apache.paimon.catalog.Identifier; import org.apache.paimon.fs.FileIO; import org.apache.paimon.fs.FileStatus; @@ -33,7 +35,9 @@ import org.apache.paimon.table.sink.CommitMessage; import org.apache.paimon.types.DataTypes; import org.apache.paimon.types.RowType; +import org.apache.paimon.utils.InstantiationUtil; import org.apache.paimon.utils.PartitionPathUtils; +import org.apache.paimon.utils.ReflectionUtils; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -75,6 +79,7 @@ import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -84,7 +89,7 @@ class FormatTableCommitTest { @TempDir java.nio.file.Path tempDir; @Test - void testPartitionRegistrationFailureDiscardsTheFilesItWrote() throws Exception { + void testPartitionRegistrationFailureSurvivesFreshCommitAbort() throws Exception { LocalFileIO fileIO = LocalFileIO.create(); Path tablePath = new Path(tempDir.toUri()); Path targetPath = new Path(tablePath, "year=2025/month=10/data-1.csv"); @@ -98,7 +103,8 @@ void testPartitionRegistrationFailureDiscardsTheFilesItWrote() throws Exception doThrow(registrationFailure) .when(partitionManager) .createPartitions(anyList(), eq(true), any(), anyBoolean()); - + Identifier identifier = + Identifier.create("catalog_partition_db", "catalog_partition_table"); FormatTableCommit commit = new FormatTableCommit( tablePath.toString(), @@ -107,25 +113,153 @@ void testPartitionRegistrationFailureDiscardsTheFilesItWrote() throws Exception false, PARTITION_DEFAULT_NAME.defaultValue(), false, - Identifier.create("catalog_partition_db", "catalog_partition_table"), + identifier, null, null, null, partitionManager, /* dynamicPartitionOverwrite */ true, /* cleanupThreadNum */ 1); - CommitMessage message = new TwoPhaseCommitMessage(committer); + TwoPhaseCommitMessage message = new TwoPhaseCommitMessage(committer); + List messages = Collections.singletonList(message); - assertThatThrownBy(() -> commit.commit(Collections.singletonList(message))) + assertThatThrownBy(() -> commit.commit(messages)) .isInstanceOf(RuntimeException.class) .hasRootCauseMessage("Catalog partition registration unavailable"); + // The catalog call's outcome is indeterminate, so abort must keep the published target. + assertThat(fileIO.exists(targetPath)).isTrue(); - // A failed write leaves nothing behind, whichever step failed: rerunning it converges, - // and an idempotent registration makes a partition that was registered anyway harmless. - assertThat(fileIO.exists(targetPath)).isFalse(); + // Spark may serialize the marked message before invoking abort on a fresh commit object. + TwoPhaseCommitMessage roundTripped = InstantiationUtil.clone(message); + assertThat(roundTripped).isNotSameAs(message); + FormatTableCommit freshAbort = + new FormatTableCommit( + tablePath.toString(), + Arrays.asList("year", "month"), + fileIO, + false, + PARTITION_DEFAULT_NAME.defaultValue(), + false, + identifier, + null, + null, + null, + partitionManager, + /* dynamicPartitionOverwrite */ true, + /* cleanupThreadNum */ 1); + freshAbort.abort(Collections.singletonList(roundTripped)); + assertThat(fileIO.exists(targetPath)).isTrue(); verify(partitionManager).createPartitions(anyList(), eq(true), any(), anyBoolean()); } + @Test + void testPartialCatalogRegistrationPreservesEveryPublishedTarget() throws Exception { + LocalFileIO fileIO = LocalFileIO.create(); + Path tablePath = new Path(tempDir.toUri()); + Identifier identifier = + Identifier.create("catalog_partition_db", "catalog_partition_table"); + Catalog catalog = mock(Catalog.class); + List> registeredPartitions = new ArrayList<>(); + AtomicInteger requests = new AtomicInteger(); + RuntimeException registrationFailure = + new RuntimeException("second catalog partition batch failed"); + doAnswer( + invocation -> { + List> batch = invocation.getArgument(1); + if (requests.getAndIncrement() == 0) { + registeredPartitions.addAll(batch); + return null; + } + throw registrationFailure; + }) + .when(catalog) + .createPartitions(eq(identifier), anyList(), eq(true), anyList(), eq(false)); + FormatTablePartitionManager partitionManager = + FormatTablePartitionManager.create( + identifier, Collections.singletonList("part"), () -> catalog); + List targetPaths = new ArrayList<>(); + List messages = new ArrayList<>(); + for (int partition = 0; partition < 1001; partition++) { + Path targetPath = new Path(tablePath, String.format("part=%04d/data.csv", partition)); + RenamingTwoPhaseOutputStream outputStream = + new RenamingTwoPhaseOutputStream(fileIO, targetPath, false); + outputStream.write(1); + targetPaths.add(targetPath); + messages.add(new TwoPhaseCommitMessage(outputStream.closeForCommit())); + } + FormatTableCommit commit = + new FormatTableCommit( + tablePath.toString(), + Collections.singletonList("part"), + fileIO, + false, + PARTITION_DEFAULT_NAME.defaultValue(), + false, + identifier, + null, + null, + null, + partitionManager, + /* dynamicPartitionOverwrite */ true, + /* cleanupThreadNum */ 1); + + assertThatThrownBy(() -> commit.commit(messages)) + .isInstanceOf(RuntimeException.class) + .hasRootCauseMessage("second catalog partition batch failed"); + + // The real manager splits this into batches of 1000, so the first request is durable. + assertThat(registeredPartitions).hasSize(1000); + verify(catalog, times(2)) + .createPartitions(eq(identifier), anyList(), eq(true), anyList(), eq(false)); + for (Path targetPath : targetPaths) { + assertThat(fileIO.exists(targetPath)).isTrue(); + } + } + + @Test + void testHivePostRegistrationFailurePreservesOverwriteTarget() throws Exception { + LocalFileIO fileIO = LocalFileIO.create(); + Path tablePath = new Path(tempDir.toUri()); + Path partitionPath = new Path(tablePath, "part=p"); + Path oldPath = new Path(partitionPath, "data-old.csv"); + Path targetPath = new Path(partitionPath, "data-new.csv"); + fileIO.writeFile(oldPath, "old", false); + RenamingTwoPhaseOutputStream outputStream = + new RenamingTwoPhaseOutputStream(fileIO, targetPath, false); + outputStream.write(1); + TwoPhaseOutputStream.Committer committer = outputStream.closeForCommit(); + Map staticPartition = Collections.singletonMap("part", "p"); + FormatTableCommit commit = + new FormatTableCommit( + tablePath.toString(), + Collections.singletonList("part"), + fileIO, + false, + PARTITION_DEFAULT_NAME.defaultValue(), + true, + Identifier.create("hive_db", "hive_table"), + staticPartition, + null, + null, + null, + /* dynamicPartitionOverwrite */ true, + /* cleanupThreadNum */ 1); + PostRegistrationFailingHiveCatalog hiveCatalog = + new PostRegistrationFailingHiveCatalog(fileIO, tablePath); + ReflectionUtils.setPrivateFieldValue(commit, "hiveCatalog", hiveCatalog); + List messages = + Collections.singletonList(new TwoPhaseCommitMessage(committer)); + + assertThatThrownBy(() -> commit.commit(messages)) + .isInstanceOf(RuntimeException.class) + .hasRootCauseMessage("Hive failed after partition registration"); + + // The replacement is the only remaining copy after Hive mutates and then fails. + assertThat(hiveCatalog.registeredPartitions).containsExactly(staticPartition); + assertThat(fileIO.exists(oldPath)).isFalse(); + assertThat(fileIO.exists(targetPath)).isTrue(); + } + @Test void testFileCommitFailureStillDiscardsUncommittedFiles() throws Exception { LocalFileIO fileIO = LocalFileIO.create(); @@ -159,6 +293,51 @@ void testFileCommitFailureStillDiscardsUncommittedFiles() throws Exception { .createPartitions(anyList(), eq(true), any(), anyBoolean()); } + @Test + void testStagingCleanupFailureDeletesPublishedFileBeforeMetadataMutation() throws Exception { + LocalFileIO fileIO = LocalFileIO.create(); + Path tablePath = new Path(tempDir.toUri()); + Path targetPath = new Path(tablePath, "part=p/data.csv"); + TwoPhaseOutputStream.Committer committer = mock(TwoPhaseOutputStream.Committer.class); + when(committer.targetPath()).thenReturn(targetPath); + doAnswer( + ignored -> { + fileIO.writeFile(targetPath, "published", false); + return null; + }) + .when(committer) + .commit(fileIO); + doThrow(new IOException("staging cleanup failed")).when(committer).clean(fileIO); + FormatTablePartitionManager partitionManager = mock(FormatTablePartitionManager.class); + FormatTableCommit commit = + new FormatTableCommit( + tablePath.toString(), + Collections.singletonList("part"), + fileIO, + false, + PARTITION_DEFAULT_NAME.defaultValue(), + false, + Identifier.create("catalog_partition_db", "catalog_partition_table"), + null, + null, + null, + partitionManager, + /* dynamicPartitionOverwrite */ true, + /* cleanupThreadNum */ 1); + + assertThatThrownBy( + () -> + commit.commit( + Collections.singletonList( + new TwoPhaseCommitMessage(committer)))) + .hasRootCauseMessage("staging cleanup failed"); + + verify(committer).discard(fileIO); + verify(partitionManager, never()) + .createPartitions(anyList(), eq(true), any(), anyBoolean()); + assertThat(fileIO.exists(targetPath)).isFalse(); + } + @Test void testAbortAttemptsEveryRollbackAndReportsDeleteFailure() throws Exception { Path tablePath = new Path(tempDir.toUri()); @@ -259,8 +438,6 @@ void testOverwriteKeepsFilesOfConcurrentWritersStagingTrees() throws Exception { fileIO.writeFile(previousDataFile, "1", false); // A concurrent job is mid-write in this partition, under a magic committer's tree. Its // file carries an ordinary data file name; only the directories above it say otherwise. - // ('_temporary' is the other such tree, but this writer's own clean() still empties it - - // covered once that is fixed separately.) Path stagingFile = new Path( partitionPath, @@ -397,8 +574,9 @@ void testOverwritingAPrefixClearsTheDefaultPartitionDirectory() throws Exception } @Test - void testPathNotMatchingThePartitionKeysFails() { + void testPathNotMatchingThePartitionKeysFails() throws Exception { Path tablePath = new Path(tempDir.toUri()); + Path targetPath = new Path(tablePath, "year=2025/day=10/data-1.csv"); // The message names the path and the declared keys, which is what tells a reader that // 'day' is not where 'month' was expected. @@ -409,6 +587,7 @@ void testPathNotMatchingThePartitionKeysFails() { .hasMessageContaining("year=2025/day=10") .hasMessageContaining("catalog_partition_db.catalog_partition_table") .hasMessageContaining("[year, month]"); + assertThat(LocalFileIO.create().exists(targetPath)).isFalse(); } @Test @@ -1818,6 +1997,23 @@ private static void collectFailures(Throwable throwable, List failure collectFailures(throwable.getCause(), failures); } + private static class PostRegistrationFailingHiveCatalog extends FileSystemCatalog { + + private final List> registeredPartitions = new ArrayList<>(); + + private PostRegistrationFailingHiveCatalog(FileIO fileIO, Path warehouse) { + super(fileIO, warehouse); + } + + public void createPartitionsUtil( + Identifier identifier, + List> partitions, + boolean partitionOnlyValueInPath) { + registeredPartitions.addAll(partitions); + throw new RuntimeException("Hive failed after partition registration"); + } + } + private static class SelectiveRefusingDeleteFileIO extends LocalFileIO { private static final long serialVersionUID = 1L; From c01de26c4a7f298b0f1aa9947725a3e024e09237 Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Thu, 27 Aug 2026 11:45:09 +0800 Subject: [PATCH 05/10] [ci] Retry flaky MySQL CDC integration test From 41fc7edbe27e66e2f02ebb010dc9bb7b3030b169 Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Thu, 27 Aug 2026 12:43:55 +0800 Subject: [PATCH 06/10] [ci] Retry flaky CDC integration tests From 9372cc5c05bb449d57f120bb275323bfe84ba4e6 Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Thu, 27 Aug 2026 22:04:10 +0800 Subject: [PATCH 07/10] [api][core] Fix metadata rollback and executor failures --- .../apache/paimon/utils/ThreadPoolUtils.java | 89 ++++---- .../paimon/utils/ThreadPoolUtilsTest.java | 212 ++++++++++++++++++ .../SemaphoredDelegatingExecutorTest.java | 112 +++++++++ .../table/format/FormatTableCommit.java | 40 +++- .../table/format/TwoPhaseCommitMessage.java | 4 +- .../FormatTableCommitStatisticsTest.java | 167 +++++++++++++- .../table/format/FormatTableCommitTest.java | 189 +++++++++++----- 7 files changed, 703 insertions(+), 110 deletions(-) create mode 100644 paimon-common/src/test/java/org/apache/paimon/utils/SemaphoredDelegatingExecutorTest.java diff --git a/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java b/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java index 3e5687aef254..3a938472a2a0 100644 --- a/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java +++ b/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java @@ -280,12 +280,11 @@ private static class SequentialBatchIterator implements CloseableBatchIter private final int queueSize; private final SchedulingMode mode; private final Queue> activeTasks = new ArrayDeque<>(); - private final Object submissionLock = new Object(); private Iterator activeResults = Collections.emptyList().iterator(); private T next; private boolean closed; - private boolean submissionStopped; + private volatile boolean submissionStopped; private SequentialBatchIterator( ExecutorService executor, @@ -349,27 +348,23 @@ private void fillWindow() { ClassLoader classLoader = Thread.currentThread().getContextClassLoader(); AccessControlContext accessControlContext = AccessController.getContext(); while (activeTasks.size() < queueSize) { - synchronized (submissionLock) { - if (submissionStopped || !input.hasNext()) { - return; - } - BatchTask task = - new BatchTask<>( - processor, - input.next(), - classLoader, - accessControlContext, - this::stopSubmission); - executor.execute(task); - activeTasks.add(task); + if (submissionStopped || !input.hasNext()) { + return; } + BatchTask task = + new BatchTask<>( + processor, + input.next(), + classLoader, + accessControlContext, + this::stopSubmission); + executor.execute(task); + activeTasks.add(task); } } private void stopSubmission() { - synchronized (submissionLock) { - submissionStopped = true; - } + submissionStopped = true; } @Override @@ -471,29 +466,47 @@ public void run() { Thread currentThread = Thread.currentThread(); boolean interruptedOnEntry = currentThread.isInterrupted(); - ClassLoader originalClassLoader = currentThread.getContextClassLoader(); + ClassLoader originalClassLoader = null; + boolean originalClassLoaderCaptured = false; try { - currentThread.setContextClassLoader(classLoader); - result = - AccessController.doPrivileged( - (PrivilegedAction>) () -> processor.apply(input), - accessControlContext); - } catch (RuntimeException | Error taskFailure) { - failure = taskFailure; - stopSubmission.run(); - } finally { - currentThread.setContextClassLoader(originalClassLoader); - synchronized (this) { - runner = null; - state = FINISHED; + try { + originalClassLoader = currentThread.getContextClassLoader(); + originalClassLoaderCaptured = true; + currentThread.setContextClassLoader(classLoader); + result = + AccessController.doPrivileged( + (PrivilegedAction>) () -> processor.apply(input), + accessControlContext); + } catch (RuntimeException | Error taskFailure) { + failure = taskFailure; + } finally { + if (originalClassLoaderCaptured) { + try { + currentThread.setContextClassLoader(originalClassLoader); + } catch (RuntimeException | Error restoreFailure) { + failure = firstOrSuppressed(restoreFailure, failure); + } + } } - // Reset the flag to its entry state so a cancelled task cannot leak an interrupt - // to a reused worker and a direct executor cannot clear its caller's interrupt. - Thread.interrupted(); - if (interruptedOnEntry) { - currentThread.interrupt(); + if (failure != null) { + stopSubmission.run(); + } + } finally { + try { + synchronized (this) { + runner = null; + state = FINISHED; + } + // Reset the flag to its entry state so a cancelled task cannot leak an + // interrupt to a reused worker or clear its caller's interrupt when using a + // direct executor. + Thread.interrupted(); + if (interruptedOnEntry) { + currentThread.interrupt(); + } + } finally { + completion.countDown(); } - completion.countDown(); } } diff --git a/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java b/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java index 30708c82d984..fec595456cce 100644 --- a/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java +++ b/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java @@ -274,6 +274,218 @@ public void testWorkerUsesCallerClassLoaderAndRestoresPoolClassLoader() throws E workers.shutdownNow(); } + @Test + public void testWorkerRestoresNullContextClassLoader() throws Exception { + ExecutorService workers = Executors.newFixedThreadPool(1); + ClassLoader callerClassLoader = new ClassLoader(getClass().getClassLoader()) {}; + ClassLoader original = Thread.currentThread().getContextClassLoader(); + AtomicReference seen = new AtomicReference<>(); + + try { + assertThat( + workers.submit( + () -> { + Thread.currentThread().setContextClassLoader(null); + return Thread.currentThread() + .getContextClassLoader(); + }) + .get(3, TimeUnit.SECONDS)) + .isNull(); + + Thread.currentThread().setContextClassLoader(callerClassLoader); + try (CloseableBatchIterator iterator = + ThreadPoolUtils.sequentialBatchedExecuteCloseable( + workers, + value -> { + seen.set(Thread.currentThread().getContextClassLoader()); + return Collections.singletonList(value); + }, + Collections.singletonList(1), + 1)) { + assertThat(iterator.next()).isOne(); + } finally { + Thread.currentThread().setContextClassLoader(original); + } + + assertThat(seen.get()).isSameAs(callerClassLoader); + assertThat( + workers.submit(() -> Thread.currentThread().getContextClassLoader()) + .get(3, TimeUnit.SECONDS)) + .isNull(); + } finally { + Thread.currentThread().setContextClassLoader(original); + workers.shutdownNow(); + assertThat(workers.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); + } + } + + @Test + public void testDeniedGetContextClassLoaderStillPublishesCompletion() throws Exception { + SecurityException getFailure = new SecurityException("get TCCL denied"); + AtomicBoolean denyContextClassLoaderAccess = new AtomicBoolean(); + AtomicInteger setAttempts = new AtomicInteger(); + AtomicBoolean processorCalled = new AtomicBoolean(); + ExecutorService workers = + Executors.newSingleThreadExecutor( + runnable -> + new Thread(runnable, "denied-get-tccl-worker") { + @Override + public ClassLoader getContextClassLoader() { + if (denyContextClassLoaderAccess.get()) { + throw getFailure; + } + return super.getContextClassLoader(); + } + + @Override + public void setContextClassLoader(ClassLoader classLoader) { + if (denyContextClassLoaderAccess.get()) { + setAttempts.incrementAndGet(); + } + super.setContextClassLoader(classLoader); + } + }); + ExecutorService consumer = Executors.newSingleThreadExecutor(); + Future taskResult = null; + try { + workers.submit(() -> {}).get(3, TimeUnit.SECONDS); + denyContextClassLoaderAccess.set(true); + + CloseableBatchIterator iterator = + ThreadPoolUtils.sequentialBatchedExecuteCloseable( + workers, + value -> { + processorCalled.set(true); + return Collections.singletonList(value); + }, + Collections.singletonList(1), + 1); + taskResult = consumer.submit(() -> catchThrowable(iterator::next)); + assertThat(taskResult.get(3, TimeUnit.SECONDS)).isSameAs(getFailure); + assertThat(processorCalled).isFalse(); + assertThat(setAttempts).hasValue(0); + consumer.submit(iterator::close).get(3, TimeUnit.SECONDS); + } finally { + denyContextClassLoaderAccess.set(false); + if (taskResult != null) { + taskResult.cancel(true); + } + consumer.shutdownNow(); + workers.shutdownNow(); + assertThat(consumer.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); + assertThat(workers.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); + } + } + + @Test + public void testRestoreContextClassLoaderFailureStillPublishesCompletion() throws Exception { + SecurityException restoreFailure = new SecurityException("restore TCCL denied"); + AtomicBoolean failContextClassLoaderRestore = new AtomicBoolean(); + AtomicInteger setAttempts = new AtomicInteger(); + AtomicBoolean processorCalled = new AtomicBoolean(); + ExecutorService workers = + Executors.newSingleThreadExecutor( + runnable -> + new Thread(runnable, "denied-restore-tccl-worker") { + @Override + public void setContextClassLoader(ClassLoader classLoader) { + if (failContextClassLoaderRestore.get() + && setAttempts.incrementAndGet() == 2) { + throw restoreFailure; + } + super.setContextClassLoader(classLoader); + } + }); + ExecutorService consumer = Executors.newSingleThreadExecutor(); + Future taskResult = null; + try { + workers.submit(() -> {}).get(3, TimeUnit.SECONDS); + failContextClassLoaderRestore.set(true); + + CloseableBatchIterator iterator = + ThreadPoolUtils.sequentialBatchedExecuteCloseable( + workers, + value -> { + processorCalled.set(true); + return Collections.singletonList(value); + }, + Collections.singletonList(1), + 1); + taskResult = consumer.submit(() -> catchThrowable(iterator::next)); + assertThat(taskResult.get(3, TimeUnit.SECONDS)).isSameAs(restoreFailure); + assertThat(processorCalled).isTrue(); + assertThat(setAttempts).hasValue(2); + consumer.submit(iterator::close).get(3, TimeUnit.SECONDS); + } finally { + failContextClassLoaderRestore.set(false); + if (taskResult != null) { + taskResult.cancel(true); + } + consumer.shutdownNow(); + workers.shutdownNow(); + assertThat(consumer.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); + assertThat(workers.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); + } + } + + @Test + public void testSetAndRestoreContextClassLoaderFailuresStillPublishCompletion() + throws Exception { + SecurityException setFailure = new SecurityException("set TCCL denied"); + SecurityException restoreFailure = new SecurityException("restore TCCL denied"); + AtomicInteger setAttempts = new AtomicInteger(); + AtomicBoolean denyContextClassLoaderChanges = new AtomicBoolean(); + ExecutorService workers = + Executors.newSingleThreadExecutor( + runnable -> + new Thread(runnable, "denied-tccl-worker") { + @Override + public void setContextClassLoader(ClassLoader classLoader) { + if (denyContextClassLoaderChanges.get()) { + throw setAttempts.incrementAndGet() == 1 + ? setFailure + : restoreFailure; + } + super.setContextClassLoader(classLoader); + } + }); + ExecutorService consumer = Executors.newSingleThreadExecutor(); + AtomicBoolean processorCalled = new AtomicBoolean(); + Future taskResult = null; + boolean taskCompleted = false; + try { + workers.submit(() -> {}).get(3, TimeUnit.SECONDS); + denyContextClassLoaderChanges.set(true); + + CloseableBatchIterator iterator = + ThreadPoolUtils.sequentialBatchedExecuteCloseable( + workers, + value -> { + processorCalled.set(true); + return Collections.singletonList(value); + }, + Collections.singletonList(1), + 1); + taskResult = consumer.submit(() -> catchThrowable(iterator::next)); + Throwable taskFailure = taskResult.get(3, TimeUnit.SECONDS); + taskCompleted = true; + + assertThat(taskFailure).isSameAs(setFailure).hasSuppressedException(restoreFailure); + assertThat(setAttempts).hasValue(2); + assertThat(processorCalled).isFalse(); + consumer.submit(iterator::close).get(3, TimeUnit.SECONDS); + } finally { + denyContextClassLoaderChanges.set(false); + if (!taskCompleted && taskResult != null) { + taskResult.cancel(true); + } + consumer.shutdownNow(); + workers.shutdownNow(); + assertThat(consumer.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); + assertThat(workers.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); + } + } + @Test public void testDirectExecutorPreservesCallerInterrupt() { ExecutorService workers = MoreExecutors.newDirectExecutorService(); diff --git a/paimon-common/src/test/java/org/apache/paimon/utils/SemaphoredDelegatingExecutorTest.java b/paimon-common/src/test/java/org/apache/paimon/utils/SemaphoredDelegatingExecutorTest.java new file mode 100644 index 000000000000..4dd0282f36aa --- /dev/null +++ b/paimon-common/src/test/java/org/apache/paimon/utils/SemaphoredDelegatingExecutorTest.java @@ -0,0 +1,112 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.paimon.utils; + +import org.apache.paimon.utils.ThreadPoolUtils.CloseableBatchIterator; + +import org.junit.jupiter.api.Test; + +import java.time.Duration; +import java.util.Arrays; +import java.util.Collections; +import java.util.Iterator; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import static org.apache.paimon.utils.CommonTestUtils.waitUtil; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.catchThrowable; + +/** Tests for {@link SemaphoredDelegatingExecutor}. */ +public class SemaphoredDelegatingExecutorTest { + + @Test + public void testFailingTaskDoesNotDeadlockBlockedSubmission() throws Exception { + ExecutorService delegated = Executors.newSingleThreadExecutor(); + SemaphoredDelegatingExecutor workers = + new SemaphoredDelegatingExecutor(delegated, 1, false); + ExecutorService consumer = Executors.newSingleThreadExecutor(); + CountDownLatch firstStarted = new CountDownLatch(1); + CountDownLatch failFirst = new CountDownLatch(1); + AtomicInteger inputsRead = new AtomicInteger(); + RuntimeException workerFailure = new RuntimeException("worker failure"); + Iterator values = Arrays.asList(0, 1, 2).iterator(); + Iterator input = + new Iterator() { + @Override + public boolean hasNext() { + return values.hasNext(); + } + + @Override + public Integer next() { + inputsRead.incrementAndGet(); + return values.next(); + } + }; + CloseableBatchIterator iterator = + ThreadPoolUtils.sequentialSlidingWindowExecuteAwaitRunningTasksOnClose( + workers, + value -> { + if (value == 0) { + firstStarted.countDown(); + await(failFirst); + throw workerFailure; + } + return Collections.singletonList(value); + }, + input, + 2); + Future result = consumer.submit(() -> catchThrowable(() -> iterator.hasNext())); + + try (CloseableBatchIterator ignored = iterator) { + try { + assertThat(firstStarted.await(3, TimeUnit.SECONDS)).isTrue(); + waitUtil( + () -> workers.getWaitingCount() == 1, + Duration.ofSeconds(3), + Duration.ofMillis(10)); + failFirst.countDown(); + + assertThat(result.get(3, TimeUnit.SECONDS)).isSameAs(workerFailure); + assertThat(inputsRead).hasValue(2); + } finally { + failFirst.countDown(); + consumer.shutdownNow(); + assertThat(consumer.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); + } + } finally { + workers.shutdownNow(); + assertThat(workers.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); + } + } + + private static void await(CountDownLatch latch) { + try { + latch.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException(e); + } + } +} diff --git a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java index c09cc89cd1b7..2efc67da0568 100644 --- a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java +++ b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java @@ -305,7 +305,7 @@ public void commit(List commitMessages) { for (TwoPhaseCommitMessage message : messages) { message.getCommitter().clean(this.fileIO); } - if (reportsStatistics) { + if (reportsStatistics && overwrite) { reportPartitions( messages, partitionSpecs, @@ -314,9 +314,9 @@ public void commit(List commitMessages) { commitTime, overwrite); } else if (partitionManager != null && !partitionSpecs.isEmpty()) { - // Concurrent writers may touch the same partition, so registration ignores the - // ones that already exist rather than failing the commit. - markPublishedTargetsToPreserveOnAbort(messages); + // Register an append before reporting its additive statistics. Registration is + // idempotent, so a failed multi-batch call can roll back every file from this + // attempt and leave any completed batches as harmless empty partition entries. partitionManager.createPartitions(new ArrayList<>(partitionSpecs), true); } boolean hiveMutationStarted = false; @@ -328,7 +328,7 @@ public void commit(List commitMessages) { } Method hiveCreatePartitionsInHmsMethod = getHiveCreatePartitionsInHmsMethod(); - if (!hiveMutationStarted) { + if (overwrite && !hiveMutationStarted) { markPublishedTargetsToPreserveOnAbort(messages); hiveMutationStarted = true; } @@ -342,6 +342,32 @@ public void commit(List commitMessages) { } } } + if (!overwrite && registersPartitions) { + // Every partition registration is now complete. A later abort must not remove + // these visible files, while a failed additive report must not make the engine + // retry the data write and add the same rows again. + markPublishedTargetsToPreserveOnAbort(messages); + if (reportsStatistics && !statisticsByPartition.isEmpty()) { + try { + reportPartitions( + messages, + partitionSpecs, + statisticsByPartition, + clearedPartitionPaths, + commitTime, + false); + } catch (RuntimeException statisticsFailure) { + LOG.warn( + "Committed data for format table {}, but failed to report append " + + "statistics for {} partitions. Run ANALYZE TABLE {} " + + "COMPUTE STATISTICS to refresh the partition statistics.", + tableIdentifier.getFullName(), + partitionSpecs.size(), + tableIdentifier.getFullName(), + statisticsFailure); + } + } + } } catch (Throwable failure) { // Cleanup restores the caller's interrupt before failing. Clear it only while aborting @@ -435,7 +461,9 @@ private void reportPartitions( } // A commit that replaced what the partitions held reports a total; an appending one saw // only its own files, so its numbers are an increment. - markPublishedTargetsToPreserveOnAbort(messages); + if (replaceStatistics) { + markPublishedTargetsToPreserveOnAbort(messages); + } partitionManager.createPartitions( new ArrayList<>(specs), true, diff --git a/paimon-core/src/main/java/org/apache/paimon/table/format/TwoPhaseCommitMessage.java b/paimon-core/src/main/java/org/apache/paimon/table/format/TwoPhaseCommitMessage.java index a8d044553abd..c936e7ccbb0a 100644 --- a/paimon-core/src/main/java/org/apache/paimon/table/format/TwoPhaseCommitMessage.java +++ b/paimon-core/src/main/java/org/apache/paimon/table/format/TwoPhaseCommitMessage.java @@ -40,8 +40,8 @@ public class TwoPhaseCommitMessage implements CommitMessage { private final long recordCount; private final long fileSizeInBytes; - // Set before external partition metadata may become durable. Keeping this state in the - // serialized message prevents a later abort instance from deleting a referenced target. + // Set once an abort must preserve the published target. Serializing the flag lets a later abort + // instance preserve it too. private boolean preservePublishedTargetOnAbort; public TwoPhaseCommitMessage(TwoPhaseOutputStream.Committer committer) { diff --git a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitStatisticsTest.java b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitStatisticsTest.java index ac857e674030..34957fa0caab 100644 --- a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitStatisticsTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitStatisticsTest.java @@ -19,6 +19,8 @@ package org.apache.paimon.table.format; import org.apache.paimon.CoreOptions; +import org.apache.paimon.catalog.Catalog; +import org.apache.paimon.catalog.CatalogLoader; import org.apache.paimon.catalog.Identifier; import org.apache.paimon.data.BinaryString; import org.apache.paimon.data.GenericRow; @@ -39,6 +41,7 @@ import org.apache.paimon.types.DataType; import org.apache.paimon.types.DataTypes; import org.apache.paimon.types.RowType; +import org.apache.paimon.utils.InstantiationUtil; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -55,6 +58,7 @@ import java.util.List; import java.util.Map; import java.util.UUID; +import java.util.concurrent.atomic.AtomicInteger; import static org.apache.paimon.CoreOptions.PARTITION_DEFAULT_NAME; import static org.assertj.core.api.Assertions.assertThat; @@ -63,6 +67,7 @@ import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyList; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -972,25 +977,125 @@ void testTheNumbersReachTheCatalogThroughTheWriteBuilder() throws Exception { } @Test - void testAFailedReportPreservesPublishedTargetWhenCatalogOutcomeIsUnknown() throws Exception { + void testAppendRegistrationLoaderFailureDeletesPublishedTarget() throws Exception { LocalFileIO fileIO = LocalFileIO.create(); Path tablePath = new Path(tempDir.toUri()); - FormatTablePartitionManager partitionManager = mock(FormatTablePartitionManager.class); - RuntimeException failure = new RuntimeException("catalog says 429"); - doThrow(failure) - .when(partitionManager) - .createPartitions(anyList(), anyBoolean(), any(), anyBoolean()); + RuntimeException failure = new RuntimeException("catalog loader failed"); + AtomicInteger loads = new AtomicInteger(); + CatalogLoader loader = + () -> { + loads.incrementAndGet(); + throw failure; + }; + FormatTablePartitionManager partitionManager = + FormatTablePartitionManager.create(TABLE, PARTITION_KEYS, loader); CommitMessage message = writtenFile(fileIO, tablePath, "year=2025/month=10", 3, 128); Path written = ((TwoPhaseCommitMessage) message).getCommitter().targetPath(); - // Once the catalog call starts, an exception cannot prove whether registration took - // effect. Removing the target could leave durable metadata pointing at a missing file. assertThatThrownBy( () -> commit(tablePath, fileIO, partitionManager, false, null) .commit(Collections.singletonList(message))) .hasRootCause(failure); + // No catalog request ran, so abort can safely delete this attempt's published file. + assertThat(loads).hasValue(1); + assertThat(fileIO.exists(written)).isFalse(); + } + + @Test + void testEmptyStaticAppendOnlyRegistersPartition() { + LocalFileIO fileIO = LocalFileIO.create(); + Path tablePath = new Path(tempDir.toUri()); + FormatTablePartitionManager partitionManager = mock(FormatTablePartitionManager.class); + Map staticPartition = spec("2025", "10"); + + commit(tablePath, fileIO, partitionManager, false, staticPartition) + .commit(Collections.emptyList()); + + verify(partitionManager).createPartitions(Collections.singletonList(staticPartition), true); + verify(partitionManager, never()) + .createPartitions(anyList(), eq(true), any(), anyBoolean()); + } + + @Test + void testAppendRegistrationBatchFailureDeletesAllTargetsWithoutReportingStatistics() + throws Exception { + LocalFileIO fileIO = LocalFileIO.create(); + Path tablePath = new Path(tempDir.toUri()); + Catalog catalog = mock(Catalog.class); + Catalog.TableNoPermissionException failure = new Catalog.TableNoPermissionException(TABLE); + AtomicInteger requests = new AtomicInteger(); + List> registered = new ArrayList<>(); + List appliedStatistics = new ArrayList<>(); + doAnswer( + invocation -> { + @SuppressWarnings("unchecked") + List> batch = invocation.getArgument(1); + @SuppressWarnings("unchecked") + List statistics = invocation.getArgument(3); + if (requests.incrementAndGet() == 2) { + // Permission is checked before the catalog mutation, so the second + // batch was not applied. + throw failure; + } + registered.addAll(batch); + if (statistics != null) { + appliedStatistics.addAll(statistics); + } + return null; + }) + .when(catalog) + .createPartitions(any(), anyList(), anyBoolean(), any(), anyBoolean()); + FormatTablePartitionManager partitionManager = + FormatTablePartitionManager.create(TABLE, PARTITION_KEYS, () -> catalog); + List messages = new ArrayList<>(); + List targets = new ArrayList<>(); + for (int i = 0; i < 1001; i++) { + CommitMessage message = + writtenFile(fileIO, tablePath, String.format("year=2025/month=%04d", i), 1, 1); + messages.add(message); + targets.add(((TwoPhaseCommitMessage) message).getCommitter().targetPath()); + } + + assertThatThrownBy( + () -> + commit(tablePath, fileIO, partitionManager, false, null) + .commit(messages)) + .hasRootCause(failure); + + assertThat(requests).hasValue(2); + assertThat(registered).hasSize(1000); + // Partition rows left by a successful registration batch are harmlessly empty. Applying + // additive statistics before every registration succeeds would instead leave them + // describing files this failed attempt rolls back. + assertThat(appliedStatistics).isEmpty(); + assertThat(targets).allSatisfy(target -> assertThat(fileIO.exists(target)).isFalse()); + } + + @Test + void testAppendStatisticsResponseLossIsNotRetriedAndDoesNotFailCommit() throws Exception { + LocalFileIO fileIO = LocalFileIO.create(); + Path tablePath = new Path(tempDir.toUri()); + ApplyingThenFailingStatisticsManager partitionManager = + new ApplyingThenFailingStatisticsManager(); + CommitMessage message = writtenFile(fileIO, tablePath, "year=2025/month=10", 3, 128); + Path written = ((TwoPhaseCommitMessage) message).getCommitter().targetPath(); + + // The catalog may have applied an additive report before its response was lost. Retrying + // that report would double count it, so the data commit succeeds after one best-effort try. + commit(tablePath, fileIO, partitionManager, false, null) + .commit(Collections.singletonList(message)); + + assertThat(partitionManager.calls).containsExactly("registration", "statistics"); + assertThat(partitionManager.statisticsAttempts).isOne(); + assertThat(partitionManager.appliedRecordCount).isEqualTo(3); + assertThat(fileIO.exists(written)).isTrue(); + + TwoPhaseCommitMessage roundTripped = + InstantiationUtil.clone((TwoPhaseCommitMessage) message); + commit(tablePath, fileIO, partitionManager, false, null) + .abort(Collections.singletonList(roundTripped)); assertThat(fileIO.exists(written)).isTrue(); } @@ -1023,6 +1128,52 @@ void testFailedOverwriteReportPreservesReplacementAfterDeletingOldData() throws assertThat(fileIO.exists(new Path(tablePath, "year=2025/month=10/old-data.csv"))).isFalse(); } + /** A catalog whose additive report takes effect before its response is lost. */ + private static class ApplyingThenFailingStatisticsManager + implements FormatTablePartitionManager { + + private static final long serialVersionUID = 1L; + + private final List calls = new ArrayList<>(); + private int statisticsAttempts; + private long appliedRecordCount; + + @Override + public void createPartitions( + List> partitions, + boolean ignoreIfExists, + @Nullable List statistics, + boolean replaceStatistics) { + if (statistics == null) { + calls.add("registration"); + return; + } + + calls.add("statistics"); + statisticsAttempts++; + for (PartitionStatistics statistic : statistics) { + appliedRecordCount += statistic.recordCount(); + } + throw new RuntimeException("statistics response lost"); + } + + @Override + public List listPartitions( + Map prefix, @Nullable Predicate filter) { + throw new UnsupportedOperationException(); + } + + @Override + public List listPartitionsByNames(List> partitions) { + throw new UnsupportedOperationException(); + } + + @Override + public void dropPartitions(List> partitions) { + throw new UnsupportedOperationException(); + } + } + /** What one call reported to the catalog. */ private static class Reported { private final List> specs; diff --git a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java index f5dc81c1f5e5..b469fb42367a 100644 --- a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java @@ -79,7 +79,6 @@ import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; -import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -89,7 +88,7 @@ class FormatTableCommitTest { @TempDir java.nio.file.Path tempDir; @Test - void testPartitionRegistrationFailureSurvivesFreshCommitAbort() throws Exception { + void testPartitionRegistrationFailureDeletesPublishedTarget() throws Exception { LocalFileIO fileIO = LocalFileIO.create(); Path tablePath = new Path(tempDir.toUri()); Path targetPath = new Path(tablePath, "year=2025/month=10/data-1.csv"); @@ -100,9 +99,7 @@ void testPartitionRegistrationFailureSurvivesFreshCommitAbort() throws Exception FormatTablePartitionManager partitionManager = mock(FormatTablePartitionManager.class); RuntimeException registrationFailure = new RuntimeException("Catalog partition registration unavailable"); - doThrow(registrationFailure) - .when(partitionManager) - .createPartitions(anyList(), eq(true), any(), anyBoolean()); + doThrow(registrationFailure).when(partitionManager).createPartitions(anyList(), eq(true)); Identifier identifier = Identifier.create("catalog_partition_db", "catalog_partition_table"); FormatTableCommit commit = @@ -126,67 +123,37 @@ void testPartitionRegistrationFailureSurvivesFreshCommitAbort() throws Exception assertThatThrownBy(() -> commit.commit(messages)) .isInstanceOf(RuntimeException.class) .hasRootCauseMessage("Catalog partition registration unavailable"); - // The catalog call's outcome is indeterminate, so abort must keep the published target. - assertThat(fileIO.exists(targetPath)).isTrue(); - - // Spark may serialize the marked message before invoking abort on a fresh commit object. - TwoPhaseCommitMessage roundTripped = InstantiationUtil.clone(message); - assertThat(roundTripped).isNotSameAs(message); - FormatTableCommit freshAbort = - new FormatTableCommit( - tablePath.toString(), - Arrays.asList("year", "month"), - fileIO, - false, - PARTITION_DEFAULT_NAME.defaultValue(), - false, - identifier, - null, - null, - null, - partitionManager, - /* dynamicPartitionOverwrite */ true, - /* cleanupThreadNum */ 1); - freshAbort.abort(Collections.singletonList(roundTripped)); - assertThat(fileIO.exists(targetPath)).isTrue(); - verify(partitionManager).createPartitions(anyList(), eq(true), any(), anyBoolean()); + assertThat(fileIO.exists(targetPath)).isFalse(); + verify(partitionManager).createPartitions(anyList(), eq(true)); + verify(partitionManager, never()) + .createPartitions(anyList(), eq(true), any(), anyBoolean()); } @Test - void testPartialCatalogRegistrationPreservesEveryPublishedTarget() throws Exception { + void testRegistrationResponseLossStillDeletesPublishedTarget() throws Exception { LocalFileIO fileIO = LocalFileIO.create(); Path tablePath = new Path(tempDir.toUri()); + Path targetPath = new Path(tablePath, "part=p/data.csv"); Identifier identifier = Identifier.create("catalog_partition_db", "catalog_partition_table"); Catalog catalog = mock(Catalog.class); List> registeredPartitions = new ArrayList<>(); - AtomicInteger requests = new AtomicInteger(); - RuntimeException registrationFailure = - new RuntimeException("second catalog partition batch failed"); + RuntimeException registrationFailure = new RuntimeException("registration response lost"); doAnswer( invocation -> { List> batch = invocation.getArgument(1); - if (requests.getAndIncrement() == 0) { - registeredPartitions.addAll(batch); - return null; - } + registeredPartitions.addAll(batch); throw registrationFailure; }) .when(catalog) - .createPartitions(eq(identifier), anyList(), eq(true), anyList(), eq(false)); + .createPartitions(eq(identifier), anyList(), eq(true), eq(null), eq(false)); FormatTablePartitionManager partitionManager = FormatTablePartitionManager.create( identifier, Collections.singletonList("part"), () -> catalog); - List targetPaths = new ArrayList<>(); - List messages = new ArrayList<>(); - for (int partition = 0; partition < 1001; partition++) { - Path targetPath = new Path(tablePath, String.format("part=%04d/data.csv", partition)); - RenamingTwoPhaseOutputStream outputStream = - new RenamingTwoPhaseOutputStream(fileIO, targetPath, false); - outputStream.write(1); - targetPaths.add(targetPath); - messages.add(new TwoPhaseCommitMessage(outputStream.closeForCommit())); - } + RenamingTwoPhaseOutputStream outputStream = + new RenamingTwoPhaseOutputStream(fileIO, targetPath, false); + outputStream.write(1); + CommitMessage message = new TwoPhaseCommitMessage(outputStream.closeForCommit()); FormatTableCommit commit = new FormatTableCommit( tablePath.toString(), @@ -203,17 +170,16 @@ void testPartialCatalogRegistrationPreservesEveryPublishedTarget() throws Except /* dynamicPartitionOverwrite */ true, /* cleanupThreadNum */ 1); - assertThatThrownBy(() -> commit.commit(messages)) + assertThatThrownBy(() -> commit.commit(Collections.singletonList(message))) .isInstanceOf(RuntimeException.class) - .hasRootCauseMessage("second catalog partition batch failed"); + .hasRootCauseMessage("registration response lost"); - // The real manager splits this into batches of 1000, so the first request is durable. - assertThat(registeredPartitions).hasSize(1000); - verify(catalog, times(2)) + // Registration without statistics is idempotent. Even if it took effect before the + // response was lost, deleting this attempt's unique file leaves a safe empty partition. + assertThat(registeredPartitions).containsExactly(Collections.singletonMap("part", "p")); + assertThat(fileIO.exists(targetPath)).isFalse(); + verify(catalog, never()) .createPartitions(eq(identifier), anyList(), eq(true), anyList(), eq(false)); - for (Path targetPath : targetPaths) { - assertThat(fileIO.exists(targetPath)).isTrue(); - } } @Test @@ -260,6 +226,101 @@ void testHivePostRegistrationFailurePreservesOverwriteTarget() throws Exception assertThat(fileIO.exists(targetPath)).isTrue(); } + @Test + void testHivePostRegistrationFailureDeletesAppendTarget() throws Exception { + LocalFileIO fileIO = LocalFileIO.create(); + Path tablePath = new Path(tempDir.toUri()); + Path targetPath = new Path(tablePath, "part=p/data-new.csv"); + RenamingTwoPhaseOutputStream outputStream = + new RenamingTwoPhaseOutputStream(fileIO, targetPath, false); + outputStream.write(1); + Map staticPartition = Collections.singletonMap("part", "p"); + FormatTableCommit commit = + new FormatTableCommit( + tablePath.toString(), + Collections.singletonList("part"), + fileIO, + false, + PARTITION_DEFAULT_NAME.defaultValue(), + false, + Identifier.create("hive_db", "hive_table"), + staticPartition, + null, + null, + null, + /* dynamicPartitionOverwrite */ true, + /* cleanupThreadNum */ 1); + PostRegistrationFailingHiveCatalog hiveCatalog = + new PostRegistrationFailingHiveCatalog(fileIO, tablePath); + ReflectionUtils.setPrivateFieldValue(commit, "hiveCatalog", hiveCatalog); + + assertThatThrownBy( + () -> + commit.commit( + Collections.singletonList( + new TwoPhaseCommitMessage( + outputStream.closeForCommit())))) + .isInstanceOf(RuntimeException.class) + .hasRootCauseMessage("Hive failed after partition registration"); + + // Hive registration is idempotent and carries no file statistics. Its empty partition may + // remain, while removing this attempt's file makes a retry safe. + assertThat(hiveCatalog.registeredPartitions).containsExactly(staticPartition); + assertThat(fileIO.exists(targetPath)).isFalse(); + } + + @Test + void testSuccessfulHiveAppendSurvivesAbortAfterMessageRoundTrip() throws Exception { + LocalFileIO fileIO = LocalFileIO.create(); + Path tablePath = new Path(tempDir.toUri()); + Path targetPath = new Path(tablePath, "part=p/data-new.csv"); + RenamingTwoPhaseOutputStream outputStream = + new RenamingTwoPhaseOutputStream(fileIO, targetPath, false); + outputStream.write(1); + TwoPhaseCommitMessage message = new TwoPhaseCommitMessage(outputStream.closeForCommit()); + Map staticPartition = Collections.singletonMap("part", "p"); + FormatTableCommit commit = + new FormatTableCommit( + tablePath.toString(), + Collections.singletonList("part"), + fileIO, + false, + PARTITION_DEFAULT_NAME.defaultValue(), + false, + Identifier.create("hive_db", "hive_table"), + staticPartition, + null, + null, + null, + /* dynamicPartitionOverwrite */ true, + /* cleanupThreadNum */ 1); + RecordingHiveCatalog hiveCatalog = new RecordingHiveCatalog(fileIO, tablePath); + ReflectionUtils.setPrivateFieldValue(commit, "hiveCatalog", hiveCatalog); + + commit.commit(Collections.singletonList(message)); + + TwoPhaseCommitMessage roundTripped = InstantiationUtil.clone(message); + FormatTableCommit abortCommit = + new FormatTableCommit( + tablePath.toString(), + Collections.singletonList("part"), + fileIO, + false, + PARTITION_DEFAULT_NAME.defaultValue(), + false, + Identifier.create("hive_db", "hive_table"), + staticPartition, + null, + null, + null, + /* dynamicPartitionOverwrite */ true, + /* cleanupThreadNum */ 1); + abortCommit.abort(Collections.singletonList(roundTripped)); + + assertThat(hiveCatalog.registeredPartitions).containsExactly(staticPartition); + assertThat(fileIO.exists(targetPath)).isTrue(); + } + @Test void testFileCommitFailureStillDiscardsUncommittedFiles() throws Exception { LocalFileIO fileIO = LocalFileIO.create(); @@ -2014,6 +2075,22 @@ public void createPartitionsUtil( } } + private static class RecordingHiveCatalog extends FileSystemCatalog { + + private final List> registeredPartitions = new ArrayList<>(); + + private RecordingHiveCatalog(FileIO fileIO, Path warehouse) { + super(fileIO, warehouse); + } + + public void createPartitionsUtil( + Identifier identifier, + List> partitions, + boolean partitionOnlyValueInPath) { + registeredPartitions.addAll(partitions); + } + } + private static class SelectiveRefusingDeleteFileIO extends LocalFileIO { private static final long serialVersionUID = 1L; From 2a39e6a5a9c93f91db07b2a70be93d362a6c7e30 Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Fri, 28 Aug 2026 02:27:51 +0800 Subject: [PATCH 08/10] [ci] Retry failed licensing build From 3cbd4b9acda875fd5b2c9e1e9750b446aa4d73d3 Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Fri, 28 Aug 2026 14:39:42 +0800 Subject: [PATCH 09/10] [api][core] Preserve overwrite replacements on commit failure --- .../fs/BaseMultiPartUploadCommitter.java | 18 +- .../paimon/fs/TwoPhaseOutputStream.java | 13 + .../table/format/FormatTableCommit.java | 43 ++-- .../table/format/FormatTableCommitTest.java | 228 ++++++++++++++++++ 4 files changed, 278 insertions(+), 24 deletions(-) diff --git a/paimon-common/src/main/java/org/apache/paimon/fs/BaseMultiPartUploadCommitter.java b/paimon-common/src/main/java/org/apache/paimon/fs/BaseMultiPartUploadCommitter.java index ffe7214de4ab..5245dcc161d4 100644 --- a/paimon-common/src/main/java/org/apache/paimon/fs/BaseMultiPartUploadCommitter.java +++ b/paimon-common/src/main/java/org/apache/paimon/fs/BaseMultiPartUploadCommitter.java @@ -71,13 +71,22 @@ public void commit(FileIO fileIO) throws IOException { @Override public void discard(FileIO fileIO) throws IOException { try { - MultiPartUploadStore multiPartUploadStore = multiPartUploadStore(fileIO); - multiPartUploadStore.abortMultipartUpload(objectName, uploadId); + abortMultipartUpload(fileIO); } catch (Exception e) { LOG.warn("Failed to discard multipart upload with ID: {}", uploadId, e); } } + @Override + public void discardStaging(FileIO fileIO) throws IOException { + try { + // Aborting an upload never deletes a possibly completed object. + abortMultipartUpload(fileIO); + } catch (Exception e) { + throw new IOException("Failed to discard multipart upload with ID: " + uploadId, e); + } + } + @Override public Path targetPath() { return this.targetPath; @@ -91,6 +100,11 @@ public List uploadedParts() { @Override public void clean(FileIO fileIO) throws IOException {} + private void abortMultipartUpload(FileIO fileIO) throws IOException { + MultiPartUploadStore multiPartUploadStore = multiPartUploadStore(fileIO); + multiPartUploadStore.abortMultipartUpload(objectName, uploadId); + } + private MultiPartUploadStore multiPartUploadStore(FileIO fileIO) throws IOException { if (fileIO instanceof RESTTokenFileIO) { RESTTokenFileIO restTokenFileIO = (RESTTokenFileIO) fileIO; diff --git a/paimon-common/src/main/java/org/apache/paimon/fs/TwoPhaseOutputStream.java b/paimon-common/src/main/java/org/apache/paimon/fs/TwoPhaseOutputStream.java index 931969ec68cb..1fa0d7cfd371 100644 --- a/paimon-common/src/main/java/org/apache/paimon/fs/TwoPhaseOutputStream.java +++ b/paimon-common/src/main/java/org/apache/paimon/fs/TwoPhaseOutputStream.java @@ -53,6 +53,19 @@ public interface Committer extends Serializable { */ void discard(FileIO fileIO) throws IOException; + /** + * Discards staged resources without deleting {@link #targetPath()}. + * + *

This is used when a commit may have taken effect and its target must therefore be + * preserved. The default delegates to {@link #clean}. Override this method if a failed or + * uncertain commit can leave staged resources that {@code clean} does not release. + * + * @throws IOException if an I/O error occurs during cleanup + */ + default void discardStaging(FileIO fileIO) throws IOException { + clean(fileIO); + } + Path targetPath(); /** diff --git a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java index 2efc67da0568..d46724e29309 100644 --- a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java +++ b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java @@ -229,6 +229,7 @@ public void commit(List commitMessages) { Set> partitionSpecs = new HashSet<>(); Set clearedPartitionPaths = new HashSet<>(); + Path staticPartitionPath = null; if (staticPartitions != null && !staticPartitions.isEmpty()) { Path partitionPath = @@ -237,6 +238,7 @@ public void commit(List commitMessages) { staticPartitions, formatTablePartitionOnlyValueInPath, partitionKeys); + staticPartitionPath = partitionPath; if (staticPartitions.size() == partitionKeys.size()) { partitionSpecs.add(staticPartitions); } @@ -249,9 +251,6 @@ public void commit(List commitMessages) { partitionKeys.size() - staticPartitions.size(), cleanupThreadNum)); } - if (!fileIO.exists(partitionPath)) { - fileIO.mkdirs(partitionPath); - } } else if (overwrite) { if (replacesOnlyWrittenPartitions()) { Set partitionPaths = new LinkedHashSet<>(); @@ -271,6 +270,14 @@ public void commit(List commitMessages) { deletePreviousDataFiles(tableDataDirectories(), 0, cleanupThreadNum)); } } + if (overwrite) { + // Old data is now permanently gone. Preserve any replacement that may become + // visible, while abort still cleans its staging resources. + markPublishedTargetsToPreserveOnAbort(messages); + } + if (staticPartitionPath != null && !fileIO.exists(staticPartitionPath)) { + fileIO.mkdirs(staticPartitionPath); + } boolean registersPartitions = partitionKeys != null @@ -307,7 +314,6 @@ public void commit(List commitMessages) { } if (reportsStatistics && overwrite) { reportPartitions( - messages, partitionSpecs, statisticsByPartition, clearedPartitionPaths, @@ -319,7 +325,6 @@ public void commit(List commitMessages) { // attempt and leave any completed batches as harmless empty partition entries. partitionManager.createPartitions(new ArrayList<>(partitionSpecs), true); } - boolean hiveMutationStarted = false; for (Map partitionSpec : partitionSpecs) { if (hiveCatalog != null) { try { @@ -328,10 +333,6 @@ public void commit(List commitMessages) { } Method hiveCreatePartitionsInHmsMethod = getHiveCreatePartitionsInHmsMethod(); - if (overwrite && !hiveMutationStarted) { - markPublishedTargetsToPreserveOnAbort(messages); - hiveMutationStarted = true; - } hiveCreatePartitionsInHmsMethod.invoke( hiveCatalog, tableIdentifier, @@ -350,7 +351,6 @@ public void commit(List commitMessages) { if (reportsStatistics && !statisticsByPartition.isEmpty()) { try { reportPartitions( - messages, partitionSpecs, statisticsByPartition, clearedPartitionPaths, @@ -439,7 +439,6 @@ private static void markPublishedTargetsToPreserveOnAbort( * reports every partition it emptied. */ private void reportPartitions( - List messages, Set> writtenPartitionSpecs, Map, PartitionStatistics> statisticsByPartition, Set clearedPartitionPaths, @@ -459,11 +458,6 @@ private void reportPartitions( if (specs.isEmpty()) { return; } - // A commit that replaced what the partitions held reports a total; an appending one saw - // only its own files, so its numbers are an increment. - if (replaceStatistics) { - markPublishedTargetsToPreserveOnAbort(messages); - } partitionManager.createPartitions( new ArrayList<>(specs), true, @@ -623,17 +617,23 @@ public void abort(List commitMessages) { } TwoPhaseCommitMessage twoPhaseCommitMessage = (TwoPhaseCommitMessage) commitMessage; - if (twoPhaseCommitMessage.shouldPreservePublishedTargetOnAbort()) { - continue; - } - TwoPhaseOutputStream.Committer committer = twoPhaseCommitMessage.getCommitter(); + boolean preservePublishedTarget = + twoPhaseCommitMessage.shouldPreservePublishedTargetOnAbort(); try { - committer.discard(fileIO); + if (preservePublishedTarget) { + committer.discardStaging(fileIO); + } else { + committer.discard(fileIO); + } } catch (Throwable discardFailure) { failure = firstOrSuppressed(discardFailure, failure); } + if (preservePublishedTarget) { + continue; + } + // FormatTableSingleFileWriter opens every target with overwrite=false. The target is // therefore owned by this write attempt, so it is safe to remove even when a remote // multipart completion took effect but its response was lost. Keep this rollback here: @@ -966,7 +966,6 @@ private void truncate(List> partitionSpecs) { // too, so the catalog stops describing files that are gone. try { reportPartitions( - Collections.emptyList(), Collections.emptySet(), emptied, clearedPartitionPaths, diff --git a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java index b469fb42367a..4453e6a5f3ad 100644 --- a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java @@ -22,8 +22,10 @@ import org.apache.paimon.catalog.Catalog; import org.apache.paimon.catalog.FileSystemCatalog; import org.apache.paimon.catalog.Identifier; +import org.apache.paimon.fs.BaseMultiPartUploadCommitter; import org.apache.paimon.fs.FileIO; import org.apache.paimon.fs.FileStatus; +import org.apache.paimon.fs.MultiPartUploadStore; import org.apache.paimon.fs.Path; import org.apache.paimon.fs.RenamingTwoPhaseOutputStream; import org.apache.paimon.fs.TwoPhaseOutputStream; @@ -399,6 +401,113 @@ void testStagingCleanupFailureDeletesPublishedFileBeforeMetadataMutation() throw assertThat(fileIO.exists(targetPath)).isFalse(); } + @Test + void testOverwriteMultipartCompletionResponseLossPreservesReplacementAndReportsAbortFailure() + throws Exception { + LocalFileIO fileIO = LocalFileIO.create(); + Path tablePath = new Path(new Path(tempDir.toUri()), "multipart-response-loss"); + Path partitionPath = new Path(tablePath, "part=p"); + Path oldPath = new Path(partitionPath, "data-old.csv"); + Path targetPath = new Path(partitionPath, "data-new.csv"); + Path stagingPath = + new Path(new Path(tempDir.toUri()), "multipart-staging/upload-in-progress"); + fileIO.writeFile(oldPath, "old", false); + fileIO.writeFile(stagingPath, "staged", false); + + @SuppressWarnings("unchecked") + MultiPartUploadStore uploadStore = mock(MultiPartUploadStore.class); + doAnswer( + invocation -> { + fileIO.writeFile(targetPath, "replacement", false); + throw new IOException("multipart completion response lost"); + }) + .when(uploadStore) + .completeMultipartUpload( + eq("part=p/data-new.csv"), + eq("upload-id"), + eq(Collections.singletonList("etag")), + eq(1L)); + doAnswer( + invocation -> { + fileIO.delete(stagingPath, false); + throw new IOException("multipart abort response lost"); + }) + .when(uploadStore) + .abortMultipartUpload(eq("part=p/data-new.csv"), eq("upload-id")); + TwoPhaseOutputStream.Committer committer = + new BaseMultiPartUploadCommitter( + "upload-id", + Collections.singletonList("etag"), + "part=p/data-new.csv", + 1L, + targetPath) { + @Override + protected MultiPartUploadStore multiPartUploadStore( + FileIO ignored, Path ignoredTarget) { + return uploadStore; + } + }; + + Throwable failure = + catchThrowable( + () -> + staticPartitionOverwriteCommit(tablePath, fileIO, 1) + .commit( + Collections.singletonList( + new TwoPhaseCommitMessage(committer)))); + + assertThat(getRootCause(failure)).hasMessage("multipart completion response lost"); + assertThat(failureTree(failure)) + .extracting(Throwable::getMessage) + .contains( + "Failed to discard multipart upload with ID: upload-id", + "multipart abort response lost"); + assertThat(fileIO.exists(oldPath)).isFalse(); + assertThat(fileIO.exists(targetPath)).isTrue(); + assertThat(fileIO.exists(stagingPath)).isFalse(); + } + + @Test + void testOverwritePostPublishCleanFailurePreservesReplacementAndRetriesStagingCleanup() + throws Exception { + LocalFileIO fileIO = LocalFileIO.create(); + Path tablePath = new Path(new Path(tempDir.toUri()), "post-publish-clean-failure"); + Path partitionPath = new Path(tablePath, "part=p"); + Path oldPath = new Path(partitionPath, "data-old.csv"); + Path targetPath = new Path(partitionPath, "data-new.csv"); + Path stagingPath = new Path(new Path(tempDir.toUri()), "clean-failure-staging/data.tmp"); + fileIO.writeFile(oldPath, "old", false); + fileIO.writeFile(stagingPath, "staged", false); + AtomicBoolean failFirstClean = new AtomicBoolean(true); + StagedFileCommitter committer = + new StagedFileCommitter(targetPath, stagingPath) { + @Override + public void commit(FileIO committingFileIO) throws IOException { + publish(committingFileIO); + } + + @Override + public void clean(FileIO cleaningFileIO) throws IOException { + if (failFirstClean.compareAndSet(true, false)) { + throw new IOException("staging cleanup failed"); + } + super.clean(cleaningFileIO); + } + }; + + assertThatThrownBy( + () -> + staticPartitionOverwriteCommit(tablePath, fileIO, 1) + .commit( + Collections.singletonList( + new TwoPhaseCommitMessage(committer)))) + .hasRootCauseMessage("staging cleanup failed"); + + assertThat(fileIO.exists(oldPath)).isFalse(); + assertThat(fileIO.exists(targetPath)).isTrue(); + assertThat(fileIO.exists(stagingPath)).isFalse(); + } + @Test void testAbortAttemptsEveryRollbackAndReportsDeleteFailure() throws Exception { Path tablePath = new Path(tempDir.toUri()); @@ -1209,6 +1318,60 @@ void testPublishFailureDrainsRunningWorkBeforeAbort() throws Exception { } } + @Test + void testOverwritePartialParallelPublishFailurePreservesReplacementAndCleansStaging() + throws Exception { + LocalFileIO fileIO = LocalFileIO.create(); + Path tablePath = new Path(new Path(tempDir.toUri()), "partial-parallel-publish"); + Path partitionPath = new Path(tablePath, "part=p"); + Path oldPath = new Path(partitionPath, "data-old.csv"); + Path successfulTarget = new Path(partitionPath, "data-success.csv"); + Path failedTarget = new Path(partitionPath, "data-failed.csv"); + Path successfulStaging = new Path(new Path(tempDir.toUri()), "partial-staging/success.tmp"); + Path failedStaging = new Path(new Path(tempDir.toUri()), "partial-staging/failed.tmp"); + fileIO.writeFile(oldPath, "old", false); + fileIO.writeFile(successfulStaging, "staged", false); + fileIO.writeFile(failedStaging, "staged", false); + + CountDownLatch bothPublishesStarted = new CountDownLatch(2); + CountDownLatch replacementPublished = new CountDownLatch(1); + StagedFileCommitter successfulCommitter = + new StagedFileCommitter(successfulTarget, successfulStaging) { + @Override + public void commit(FileIO committingFileIO) throws IOException { + bothPublishesStarted.countDown(); + awaitLatch(bothPublishesStarted, "both overwrite publications to start"); + publish(committingFileIO); + replacementPublished.countDown(); + } + }; + StagedFileCommitter failingCommitter = + new StagedFileCommitter(failedTarget, failedStaging) { + @Override + public void commit(FileIO committingFileIO) throws IOException { + bothPublishesStarted.countDown(); + awaitLatch(bothPublishesStarted, "both overwrite publications to start"); + awaitLatch(replacementPublished, "the parallel replacement publication"); + publish(committingFileIO); + throw new IOException("parallel publish failed"); + } + }; + List messages = + Arrays.asList( + new TwoPhaseCommitMessage(successfulCommitter), + new TwoPhaseCommitMessage(failingCommitter)); + + assertThatThrownBy( + () -> staticPartitionOverwriteCommit(tablePath, fileIO, 2).commit(messages)) + .hasRootCauseMessage("parallel publish failed"); + + assertThat(fileIO.exists(oldPath)).isFalse(); + assertThat(fileIO.exists(successfulTarget)).isTrue(); + assertThat(fileIO.exists(failedTarget)).isTrue(); + assertThat(fileIO.exists(successfulStaging)).isFalse(); + assertThat(fileIO.exists(failedStaging)).isFalse(); + } + @Test void testPublishConcurrencyIsGatedToCatalogManagedPartitionedTables() throws Exception { LocalFileIO fileIO = LocalFileIO.create(); @@ -1872,6 +2035,25 @@ private FormatTableCommit overwritingCommit( /* cleanupThreadNum */ 1); } + private FormatTableCommit staticPartitionOverwriteCommit( + Path tableLocation, FileIO fileIO, int publishThreadNum) { + return new FormatTableCommit( + tableLocation.toString(), + Collections.singletonList("part"), + fileIO, + false, + PARTITION_DEFAULT_NAME.defaultValue(), + true, + Identifier.create("overwrite_db", "overwrite_table"), + Collections.singletonMap("part", "p"), + null, + null, + null, + /* dynamicPartitionOverwrite */ true, + /* cleanupThreadNum */ 1, + publishThreadNum); + } + private void assertOverwriteCleanupSpansPartitions(boolean dynamicPartitionOverwrite) throws Exception { ParallelDeleteFileIO fileIO = new ParallelDeleteFileIO(4); @@ -2041,6 +2223,17 @@ private static ExecutionException awaitFailure(Future future) throws Exceptio } } + private static void awaitLatch(CountDownLatch latch, String description) throws IOException { + try { + if (!latch.await(10, TimeUnit.SECONDS)) { + throw new IOException("Timed out waiting for " + description); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IOException("Interrupted while waiting for " + description, e); + } + } + private static List failureTree(Throwable throwable) { List failures = new ArrayList<>(); collectFailures(throwable, failures); @@ -2058,6 +2251,41 @@ private static void collectFailures(Throwable throwable, List failure collectFailures(throwable.getCause(), failures); } + private abstract static class StagedFileCommitter implements TwoPhaseOutputStream.Committer { + + private static final long serialVersionUID = 1L; + + private final Path targetPath; + private final Path stagingPath; + + private StagedFileCommitter(Path targetPath, Path stagingPath) { + this.targetPath = targetPath; + this.stagingPath = stagingPath; + } + + protected void publish(FileIO fileIO) throws IOException { + fileIO.writeFile(targetPath, "replacement", false); + } + + @Override + public void discard(FileIO fileIO) { + fileIO.deleteQuietly(targetPath); + fileIO.deleteQuietly(stagingPath); + } + + @Override + public Path targetPath() { + return targetPath; + } + + @Override + public void clean(FileIO fileIO) throws IOException { + if (!fileIO.delete(stagingPath, false) && fileIO.exists(stagingPath)) { + throw new IOException("Failed to clean staging file " + stagingPath); + } + } + } + private static class PostRegistrationFailingHiveCatalog extends FileSystemCatalog { private final List> registeredPartitions = new ArrayList<>(); From 4c0e933e18d268e47581d07592185ece2405fc21 Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Fri, 28 Aug 2026 17:30:05 +0800 Subject: [PATCH 10/10] [core][flink] Address format commit review feedback --- .../apache/paimon/utils/ThreadPoolUtils.java | 218 ++----- .../paimon/utils/ThreadPoolUtilsTest.java | 588 +----------------- .../SemaphoredDelegatingExecutorTest.java | 112 ---- .../table/format/FormatTableCommit.java | 309 +++++++-- .../paimon/utils/ManifestReadThreadPool.java | 2 +- .../FormatTableCommitStatisticsTest.java | 3 +- .../table/format/FormatTableCommitTest.java | 362 +++++++++-- .../sink/FlinkFormatTableDataStreamSink.java | 8 +- .../FlinkFormatTableDataStreamSinkTest.java | 57 ++ 9 files changed, 725 insertions(+), 934 deletions(-) delete mode 100644 paimon-common/src/test/java/org/apache/paimon/utils/SemaphoredDelegatingExecutorTest.java diff --git a/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java b/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java index 3a938472a2a0..b5c28a19a08a 100644 --- a/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java +++ b/paimon-api/src/main/java/org/apache/paimon/utils/ThreadPoolUtils.java @@ -21,9 +21,6 @@ import org.apache.paimon.shade.guava30.com.google.common.collect.Iterators; import org.apache.paimon.shade.guava30.com.google.common.collect.Lists; -import java.security.AccessControlContext; -import java.security.AccessController; -import java.security.PrivilegedAction; import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collection; @@ -50,7 +47,7 @@ /** Utils for thread pool. */ public class ThreadPoolUtils { - /** An iterator which waits for its active tasks to quiesce when closed. */ + /** An iterator which waits for its active batch to quiesce when closed. */ public interface CloseableBatchIterator extends Iterator, AutoCloseable { @Override @@ -137,55 +134,19 @@ private void advanceIfNeeded() { } /** - * Processes one bounded batch at a time and returns results in input order. + * Parallel processes one bounded batch at a time and returns results in input order. * - *

Closing cancels unstarted tasks, interrupts running tasks, and waits for every submitted - * task to finish. + *

The caller must close the iterator to cancel unstarted tasks and wait for running tasks. */ public static CloseableBatchIterator sequentialBatchedExecuteCloseable( ExecutorService executor, Function> processor, List input, int queueSize) { - return newSequentialBatchIterator( - executor, - processor, - input.iterator(), - queueSize, - SchedulingMode.BATCHED_CANCEL_RUNNING); - } - - /** - * Processes a bounded sliding window of inputs from the iterator and returns results in input - * order. - * - *

Unlike {@link #sequentialBatchedExecuteCloseable}, closing waits for a task that has - * already started instead of interrupting it. - * - *

Use this when a task changes stored state. Interrupting a delete or a write halfway leaves - * the caller unable to say whether it took effect, so a caller that has to know the outcome of - * everything it handed out cannot let close cancel work that is already running. - */ - public static - CloseableBatchIterator sequentialSlidingWindowExecuteAwaitRunningTasksOnClose( - ExecutorService executor, - Function> processor, - Iterator input, - int queueSize) { - return newSequentialBatchIterator( - executor, processor, input, queueSize, SchedulingMode.SLIDING_AWAIT_RUNNING); - } - - private static CloseableBatchIterator newSequentialBatchIterator( - ExecutorService executor, - Function> processor, - Iterator input, - int queueSize, - SchedulingMode mode) { if (queueSize <= 0) { throw new NegativeArraySizeException("queue size should not be negative"); } - return new SequentialBatchIterator<>(executor, processor, input, queueSize, mode); + return new SequentialBatchIterator<>(executor, processor, input, queueSize); } public static void randomlyOnlyExecute( @@ -259,44 +220,25 @@ public static void awaitAllFutures(List> futures) { } } - private enum SchedulingMode { - BATCHED_CANCEL_RUNNING(true, false), - SLIDING_AWAIT_RUNNING(false, true); - - private final boolean cancelRunningOnClose; - private final boolean slidingWindow; - - SchedulingMode(boolean cancelRunningOnClose, boolean slidingWindow) { - this.cancelRunningOnClose = cancelRunningOnClose; - this.slidingWindow = slidingWindow; - } - } - private static class SequentialBatchIterator implements CloseableBatchIterator { private final ExecutorService executor; private final Function> processor; - private final Iterator input; - private final int queueSize; - private final SchedulingMode mode; + private final Queue> batches; private final Queue> activeTasks = new ArrayDeque<>(); private Iterator activeResults = Collections.emptyList().iterator(); private T next; private boolean closed; - private volatile boolean submissionStopped; private SequentialBatchIterator( ExecutorService executor, Function> processor, - Iterator input, - int queueSize, - SchedulingMode mode) { + List input, + int queueSize) { this.executor = executor; this.processor = processor; - this.input = input; - this.queueSize = queueSize; - this.mode = mode; + this.batches = new ArrayDeque<>(Lists.partition(input, queueSize)); } @Override @@ -321,77 +263,52 @@ private void advanceIfNeeded() { while (next == null) { if (activeResults.hasNext()) { next = activeResults.next(); - continue; - } - if (mode.slidingWindow || activeTasks.isEmpty()) { - fillWindow(); - } - if (activeTasks.isEmpty()) { - return; - } - BatchTask task = activeTasks.peek(); - try { - List results = task.result(); - activeTasks.poll(); - activeResults = results.iterator(); - } catch (RuntimeException | Error failure) { - if (task.failureReported()) { + } else if (!activeTasks.isEmpty()) { + BatchTask task = activeTasks.peek(); + try { + List results = task.result(); activeTasks.poll(); + activeResults = results.iterator(); + } catch (RuntimeException | Error failure) { + if (task.failureReported()) { + activeTasks.poll(); + } + throw failure; } - throw failure; + } else if (batches.isEmpty()) { + return; + } else { + submitBatch(batches.poll()); } } } - /** Does not consume more input than the active-task window can hold. */ - private void fillWindow() { + private void submitBatch(List batch) { ClassLoader classLoader = Thread.currentThread().getContextClassLoader(); - AccessControlContext accessControlContext = AccessController.getContext(); - while (activeTasks.size() < queueSize) { - if (submissionStopped || !input.hasNext()) { - return; - } - BatchTask task = - new BatchTask<>( - processor, - input.next(), - classLoader, - accessControlContext, - this::stopSubmission); + for (U input : batch) { + BatchTask task = new BatchTask<>(processor, input, classLoader); executor.execute(task); activeTasks.add(task); } } - private void stopSubmission() { - submissionStopped = true; - } - @Override public synchronized void close() { if (closed) { return; } closed = true; + batches.clear(); Throwable failure = null; boolean interrupted = Thread.interrupted(); for (BatchTask task : activeTasks) { try { - task.cancelIfUnstarted(); + task.cancel(); } catch (Throwable cleanupFailure) { failure = firstOrSuppressed(cleanupFailure, failure); } } - if (mode.cancelRunningOnClose) { - for (BatchTask task : activeTasks) { - try { - task.interruptIfRunning(); - } catch (Throwable cleanupFailure) { - failure = firstOrSuppressed(cleanupFailure, failure); - } - } - } for (BatchTask task : activeTasks) { while (true) { try { @@ -429,8 +346,6 @@ private static class BatchTask implements Runnable { private final Function> processor; private final U input; private final ClassLoader classLoader; - private final AccessControlContext accessControlContext; - private final Runnable stopSubmission; private final CountDownLatch completion = new CountDownLatch(1); private int state = CREATED; @@ -439,17 +354,10 @@ private static class BatchTask implements Runnable { private Throwable failure; private volatile boolean failureReported; - private BatchTask( - Function> processor, - U input, - ClassLoader classLoader, - AccessControlContext accessControlContext, - Runnable stopSubmission) { + private BatchTask(Function> processor, U input, ClassLoader classLoader) { this.processor = processor; this.input = input; this.classLoader = classLoader; - this.accessControlContext = accessControlContext; - this.stopSubmission = stopSubmission; } @Override @@ -464,73 +372,35 @@ public void run() { runner = Thread.currentThread(); } - Thread currentThread = Thread.currentThread(); - boolean interruptedOnEntry = currentThread.isInterrupted(); - ClassLoader originalClassLoader = null; - boolean originalClassLoaderCaptured = false; try { - try { - originalClassLoader = currentThread.getContextClassLoader(); - originalClassLoaderCaptured = true; - currentThread.setContextClassLoader(classLoader); - result = - AccessController.doPrivileged( - (PrivilegedAction>) () -> processor.apply(input), - accessControlContext); - } catch (RuntimeException | Error taskFailure) { - failure = taskFailure; - } finally { - if (originalClassLoaderCaptured) { - try { - currentThread.setContextClassLoader(originalClassLoader); - } catch (RuntimeException | Error restoreFailure) { - failure = firstOrSuppressed(restoreFailure, failure); - } - } - } - if (failure != null) { - stopSubmission.run(); - } + Thread.currentThread().setContextClassLoader(classLoader); + result = processor.apply(input); + } catch (RuntimeException | Error taskFailure) { + failure = taskFailure; } finally { - try { - synchronized (this) { - runner = null; - state = FINISHED; - } - // Reset the flag to its entry state so a cancelled task cannot leak an - // interrupt to a reused worker or clear its caller's interrupt when using a - // direct executor. - Thread.interrupted(); - if (interruptedOnEntry) { - currentThread.interrupt(); - } - } finally { - completion.countDown(); + synchronized (this) { + runner = null; + state = FINISHED; } + completion.countDown(); } } - private synchronized void interruptIfRunning() { - if (state == RUNNING) { - runner.interrupt(); - } - } - - private synchronized void cancelIfUnstarted() { + private synchronized void cancel() { if (state == CREATED) { state = CANCELLED; completion.countDown(); + } else if (state == RUNNING) { + runner.interrupt(); } } private List result() { - if (completion.getCount() != 0) { - try { - completion.await(); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - throw new RuntimeException(e); - } + try { + completion.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException(e); } if (failure != null) { failureReported = true; diff --git a/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java b/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java index fec595456cce..4bb556343f55 100644 --- a/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java +++ b/paimon-api/src/test/java/org/apache/paimon/utils/ThreadPoolUtilsTest.java @@ -20,20 +20,11 @@ import org.apache.paimon.utils.ThreadPoolUtils.CloseableBatchIterator; -import org.apache.paimon.shade.guava30.com.google.common.collect.Iterators; -import org.apache.paimon.shade.guava30.com.google.common.util.concurrent.MoreExecutors; - import org.junit.jupiter.api.Test; -import javax.security.auth.Subject; - -import java.security.AccessController; -import java.security.PrivilegedAction; -import java.util.AbstractList; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; -import java.util.Iterator; import java.util.List; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; @@ -54,26 +45,11 @@ public class ThreadPoolUtilsTest { @Test public void testCloseableBatchReturnsInOrderAndBoundsSubmission() throws Exception { - ThreadPoolExecutor workers = (ThreadPoolExecutor) Executors.newFixedThreadPool(2); + CountingThreadPoolExecutor workers = new CountingThreadPoolExecutor(2); ExecutorService consumer = Executors.newSingleThreadExecutor(); CountDownLatch firstStarted = new CountDownLatch(1); CountDownLatch secondFinished = new CountDownLatch(1); - CountDownLatch thirdStarted = new CountDownLatch(1); CountDownLatch releaseFirst = new CountDownLatch(1); - AtomicInteger inputsRead = new AtomicInteger(); - List inputs = - new AbstractList() { - @Override - public Integer get(int index) { - inputsRead.incrementAndGet(); - return index; - } - - @Override - public int size() { - return 4; - } - }; CloseableBatchIterator iterator = ThreadPoolUtils.sequentialBatchedExecuteCloseable( workers, @@ -83,12 +59,10 @@ public int size() { await(releaseFirst); } else if (input == 1) { secondFinished.countDown(); - } else if (input == 2) { - thirdStarted.countDown(); } return Collections.singletonList(input); }, - inputs, + Arrays.asList(0, 1, 2, 3), 2); try { @@ -101,22 +75,18 @@ public int size() { assertThat(firstStarted.await(3, TimeUnit.SECONDS)).isTrue(); assertThat(secondFinished.await(3, TimeUnit.SECONDS)).isTrue(); - assertThat(inputsRead).hasValue(2); - assertThat(thirdStarted.getCount()).isOne(); + assertThat(workers.getSubmittedTaskCount()).isEqualTo(2); assertThat(firstResult.isDone()).isFalse(); releaseFirst.countDown(); List results = new ArrayList<>(); results.add(firstResult.get(3, TimeUnit.SECONDS)); - // Reading the remaining result must not submit the next batch. assertThat(iterator.hasNext()).isTrue(); - assertThat(inputsRead).hasValue(2); - assertThat(thirdStarted.getCount()).isOne(); results.add(iterator.next()); + assertThat(workers.getSubmittedTaskCount()).isEqualTo(2); - // The next lookup starts the second batch only after the first batch is drained. assertThat(iterator.hasNext()).isTrue(); - assertThat(thirdStarted.await(3, TimeUnit.SECONDS)).isTrue(); + assertThat(workers.getSubmittedTaskCount()).isEqualTo(4); results.add(iterator.next()); assertThat(iterator.hasNext()).isTrue(); results.add(iterator.next()); @@ -132,443 +102,9 @@ public int size() { } } - @Test - public void testLazyInputIsConsumedOnlyAsSlotsFree() throws Exception { - ExecutorService workers = Executors.newFixedThreadPool(2); - CountDownLatch releaseFirst = new CountDownLatch(1); - AtomicInteger inputsRead = new AtomicInteger(); - Iterator input = - new Iterator() { - private int next; - - @Override - public boolean hasNext() { - return next < 100; - } - - @Override - public Integer next() { - inputsRead.incrementAndGet(); - return next++; - } - }; - try (CloseableBatchIterator iterator = - ThreadPoolUtils.sequentialSlidingWindowExecuteAwaitRunningTasksOnClose( - workers, - value -> { - if (value == 0) { - await(releaseFirst); - } - return Collections.singletonList(value); - }, - input, - 4)) { - releaseFirst.countDown(); - assertThat(iterator.next()).isEqualTo(0); - assertThat(inputsRead).hasValue(4); - assertThat(iterator.next()).isEqualTo(1); - assertThat(inputsRead).hasValue(5); - } finally { - workers.shutdownNow(); - } - } - - @Test - public void testWorkerFailureStopsNewSubmissions() throws Exception { - ExecutorService workers = Executors.newFixedThreadPool(2); - ExecutorService consumer = Executors.newSingleThreadExecutor(); - CountDownLatch secondFailed = new CountDownLatch(1); - CountDownLatch releaseFirst = new CountDownLatch(1); - AtomicInteger inputsRead = new AtomicInteger(); - RuntimeException workerFailure = new RuntimeException("worker failure"); - Iterator input = - Iterators.transform( - Arrays.asList(0, 1, 2).iterator(), - value -> { - inputsRead.incrementAndGet(); - return value; - }); - CloseableBatchIterator iterator = - ThreadPoolUtils.sequentialSlidingWindowExecuteAwaitRunningTasksOnClose( - workers, - value -> { - if (value == 0) { - await(releaseFirst); - } else if (value == 1) { - secondFailed.countDown(); - throw workerFailure; - } - return Collections.singletonList(value); - }, - input, - 2); - - try { - Future result = - consumer.submit( - () -> - catchThrowable( - () -> { - assertThat(iterator.next()).isZero(); - iterator.hasNext(); - })); - - assertThat(secondFailed.await(3, TimeUnit.SECONDS)).isTrue(); - // With input 0 still gated, this can only run after the failed task has - // completely left BatchTask.run and published its failure state. - workers.submit(() -> {}).get(3, TimeUnit.SECONDS); - assertThat(inputsRead).hasValue(2); - - releaseFirst.countDown(); - assertThat(result.get(3, TimeUnit.SECONDS)).isSameAs(workerFailure); - assertThat(inputsRead).hasValue(2); - } finally { - releaseFirst.countDown(); - iterator.close(); - consumer.shutdownNow(); - workers.shutdownNow(); - assertThat(consumer.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); - assertThat(workers.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); - } - } - - @Test - public void testWorkerUsesCallerClassLoaderAndRestoresPoolClassLoader() throws Exception { - ExecutorService workers = Executors.newFixedThreadPool(1); - ClassLoader callerClassLoader = new ClassLoader(getClass().getClassLoader()) {}; - AtomicReference poolClassLoader = new AtomicReference<>(); - workers.submit(() -> poolClassLoader.set(Thread.currentThread().getContextClassLoader())) - .get(10, TimeUnit.SECONDS); - - ClassLoader original = Thread.currentThread().getContextClassLoader(); - List seen = new ArrayList<>(); - try { - Thread.currentThread().setContextClassLoader(callerClassLoader); - try (CloseableBatchIterator iterator = - ThreadPoolUtils.sequentialBatchedExecuteCloseable( - workers, - value -> { - seen.add(Thread.currentThread().getContextClassLoader()); - return Collections.singletonList(value); - }, - Arrays.asList(0, 1), - 1)) { - while (iterator.hasNext()) { - iterator.next(); - } - } - } finally { - Thread.currentThread().setContextClassLoader(original); - } - - assertThat(seen).containsExactly(callerClassLoader, callerClassLoader); - // The pool is shared, so a worker that keeps a caller's loader would hand it to whatever - // runs on that thread next. - AtomicReference restoredPoolClassLoader = new AtomicReference<>(); - workers.submit( - () -> - restoredPoolClassLoader.set( - Thread.currentThread().getContextClassLoader())) - .get(10, TimeUnit.SECONDS); - assertThat(restoredPoolClassLoader.get()).isSameAs(poolClassLoader.get()); - workers.shutdownNow(); - } - - @Test - public void testWorkerRestoresNullContextClassLoader() throws Exception { - ExecutorService workers = Executors.newFixedThreadPool(1); - ClassLoader callerClassLoader = new ClassLoader(getClass().getClassLoader()) {}; - ClassLoader original = Thread.currentThread().getContextClassLoader(); - AtomicReference seen = new AtomicReference<>(); - - try { - assertThat( - workers.submit( - () -> { - Thread.currentThread().setContextClassLoader(null); - return Thread.currentThread() - .getContextClassLoader(); - }) - .get(3, TimeUnit.SECONDS)) - .isNull(); - - Thread.currentThread().setContextClassLoader(callerClassLoader); - try (CloseableBatchIterator iterator = - ThreadPoolUtils.sequentialBatchedExecuteCloseable( - workers, - value -> { - seen.set(Thread.currentThread().getContextClassLoader()); - return Collections.singletonList(value); - }, - Collections.singletonList(1), - 1)) { - assertThat(iterator.next()).isOne(); - } finally { - Thread.currentThread().setContextClassLoader(original); - } - - assertThat(seen.get()).isSameAs(callerClassLoader); - assertThat( - workers.submit(() -> Thread.currentThread().getContextClassLoader()) - .get(3, TimeUnit.SECONDS)) - .isNull(); - } finally { - Thread.currentThread().setContextClassLoader(original); - workers.shutdownNow(); - assertThat(workers.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); - } - } - - @Test - public void testDeniedGetContextClassLoaderStillPublishesCompletion() throws Exception { - SecurityException getFailure = new SecurityException("get TCCL denied"); - AtomicBoolean denyContextClassLoaderAccess = new AtomicBoolean(); - AtomicInteger setAttempts = new AtomicInteger(); - AtomicBoolean processorCalled = new AtomicBoolean(); - ExecutorService workers = - Executors.newSingleThreadExecutor( - runnable -> - new Thread(runnable, "denied-get-tccl-worker") { - @Override - public ClassLoader getContextClassLoader() { - if (denyContextClassLoaderAccess.get()) { - throw getFailure; - } - return super.getContextClassLoader(); - } - - @Override - public void setContextClassLoader(ClassLoader classLoader) { - if (denyContextClassLoaderAccess.get()) { - setAttempts.incrementAndGet(); - } - super.setContextClassLoader(classLoader); - } - }); - ExecutorService consumer = Executors.newSingleThreadExecutor(); - Future taskResult = null; - try { - workers.submit(() -> {}).get(3, TimeUnit.SECONDS); - denyContextClassLoaderAccess.set(true); - - CloseableBatchIterator iterator = - ThreadPoolUtils.sequentialBatchedExecuteCloseable( - workers, - value -> { - processorCalled.set(true); - return Collections.singletonList(value); - }, - Collections.singletonList(1), - 1); - taskResult = consumer.submit(() -> catchThrowable(iterator::next)); - assertThat(taskResult.get(3, TimeUnit.SECONDS)).isSameAs(getFailure); - assertThat(processorCalled).isFalse(); - assertThat(setAttempts).hasValue(0); - consumer.submit(iterator::close).get(3, TimeUnit.SECONDS); - } finally { - denyContextClassLoaderAccess.set(false); - if (taskResult != null) { - taskResult.cancel(true); - } - consumer.shutdownNow(); - workers.shutdownNow(); - assertThat(consumer.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); - assertThat(workers.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); - } - } - - @Test - public void testRestoreContextClassLoaderFailureStillPublishesCompletion() throws Exception { - SecurityException restoreFailure = new SecurityException("restore TCCL denied"); - AtomicBoolean failContextClassLoaderRestore = new AtomicBoolean(); - AtomicInteger setAttempts = new AtomicInteger(); - AtomicBoolean processorCalled = new AtomicBoolean(); - ExecutorService workers = - Executors.newSingleThreadExecutor( - runnable -> - new Thread(runnable, "denied-restore-tccl-worker") { - @Override - public void setContextClassLoader(ClassLoader classLoader) { - if (failContextClassLoaderRestore.get() - && setAttempts.incrementAndGet() == 2) { - throw restoreFailure; - } - super.setContextClassLoader(classLoader); - } - }); - ExecutorService consumer = Executors.newSingleThreadExecutor(); - Future taskResult = null; - try { - workers.submit(() -> {}).get(3, TimeUnit.SECONDS); - failContextClassLoaderRestore.set(true); - - CloseableBatchIterator iterator = - ThreadPoolUtils.sequentialBatchedExecuteCloseable( - workers, - value -> { - processorCalled.set(true); - return Collections.singletonList(value); - }, - Collections.singletonList(1), - 1); - taskResult = consumer.submit(() -> catchThrowable(iterator::next)); - assertThat(taskResult.get(3, TimeUnit.SECONDS)).isSameAs(restoreFailure); - assertThat(processorCalled).isTrue(); - assertThat(setAttempts).hasValue(2); - consumer.submit(iterator::close).get(3, TimeUnit.SECONDS); - } finally { - failContextClassLoaderRestore.set(false); - if (taskResult != null) { - taskResult.cancel(true); - } - consumer.shutdownNow(); - workers.shutdownNow(); - assertThat(consumer.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); - assertThat(workers.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); - } - } - - @Test - public void testSetAndRestoreContextClassLoaderFailuresStillPublishCompletion() - throws Exception { - SecurityException setFailure = new SecurityException("set TCCL denied"); - SecurityException restoreFailure = new SecurityException("restore TCCL denied"); - AtomicInteger setAttempts = new AtomicInteger(); - AtomicBoolean denyContextClassLoaderChanges = new AtomicBoolean(); - ExecutorService workers = - Executors.newSingleThreadExecutor( - runnable -> - new Thread(runnable, "denied-tccl-worker") { - @Override - public void setContextClassLoader(ClassLoader classLoader) { - if (denyContextClassLoaderChanges.get()) { - throw setAttempts.incrementAndGet() == 1 - ? setFailure - : restoreFailure; - } - super.setContextClassLoader(classLoader); - } - }); - ExecutorService consumer = Executors.newSingleThreadExecutor(); - AtomicBoolean processorCalled = new AtomicBoolean(); - Future taskResult = null; - boolean taskCompleted = false; - try { - workers.submit(() -> {}).get(3, TimeUnit.SECONDS); - denyContextClassLoaderChanges.set(true); - - CloseableBatchIterator iterator = - ThreadPoolUtils.sequentialBatchedExecuteCloseable( - workers, - value -> { - processorCalled.set(true); - return Collections.singletonList(value); - }, - Collections.singletonList(1), - 1); - taskResult = consumer.submit(() -> catchThrowable(iterator::next)); - Throwable taskFailure = taskResult.get(3, TimeUnit.SECONDS); - taskCompleted = true; - - assertThat(taskFailure).isSameAs(setFailure).hasSuppressedException(restoreFailure); - assertThat(setAttempts).hasValue(2); - assertThat(processorCalled).isFalse(); - consumer.submit(iterator::close).get(3, TimeUnit.SECONDS); - } finally { - denyContextClassLoaderChanges.set(false); - if (!taskCompleted && taskResult != null) { - taskResult.cancel(true); - } - consumer.shutdownNow(); - workers.shutdownNow(); - assertThat(consumer.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); - assertThat(workers.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); - } - } - - @Test - public void testDirectExecutorPreservesCallerInterrupt() { - ExecutorService workers = MoreExecutors.newDirectExecutorService(); - try { - Thread.currentThread().interrupt(); - try (CloseableBatchIterator iterator = - ThreadPoolUtils.sequentialBatchedExecuteCloseable( - workers, Collections::singletonList, Collections.singletonList(1), 1)) { - assertThat(iterator.next()).isOne(); - assertThat(Thread.currentThread().isInterrupted()).isTrue(); - } - } finally { - Thread.interrupted(); - workers.shutdownNow(); - } - } - - @Test - public void testWorkerRunsWithTheSubmittingSubject() throws Exception { - ExecutorService workers = - ThreadPoolUtils.createCachedThreadPool(1, "subject-propagation-test"); - Subject firstSubject = new Subject(); - Subject secondSubject = new Subject(); - List seenSubjects = new ArrayList<>(); - List seenWorkers = new ArrayList<>(); - PrivilegedAction readSubjectFromWorker = - () -> { - try (CloseableBatchIterator iterator = - ThreadPoolUtils.sequentialBatchedExecuteCloseable( - workers, - ignored -> { - seenWorkers.add(Thread.currentThread()); - return Collections.singletonList( - Subject.getSubject(AccessController.getContext())); - }, - Collections.singletonList(0), - 1)) { - return iterator.next(); - } - }; - - try { - for (Subject subject : Arrays.asList(firstSubject, secondSubject)) { - seenSubjects.add(Subject.doAs(subject, readSubjectFromWorker)); - } - - assertThat(seenWorkers.get(1)).isSameAs(seenWorkers.get(0)); - assertThat(seenSubjects.get(0)).isSameAs(firstSubject); - assertThat(seenSubjects.get(1)).isSameAs(secondSubject); - } finally { - workers.shutdownNow(); - assertThat(workers.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); - } - } - @Test public void testCloseCancelsQueuedTasksAndWaitsUninterruptibly() throws Exception { - LinkedBlockingQueue taskQueue = new LinkedBlockingQueue<>(); - AtomicBoolean runQueuedTaskOnInterrupt = new AtomicBoolean(); - CountDownLatch interruptHookFinished = new CountDownLatch(1); - // If close interrupts the worker before cancelling queued tasks, interrupt() runs the - // queued task and exposes the ordering bug. - ThreadPoolExecutor workers = - new ThreadPoolExecutor( - 1, - 1, - 0L, - TimeUnit.MILLISECONDS, - taskQueue, - runnable -> - new Thread(runnable) { - @Override - public void interrupt() { - super.interrupt(); - if (runQueuedTaskOnInterrupt.compareAndSet(true, false)) { - Runnable queuedTask = taskQueue.poll(); - if (queuedTask != null) { - queuedTask.run(); - } - interruptHookFinished.countDown(); - } - } - }); + ThreadPoolExecutor workers = (ThreadPoolExecutor) Executors.newFixedThreadPool(1); ExecutorService closer = Executors.newSingleThreadExecutor(); CountDownLatch secondStarted = new CountDownLatch(1); CountDownLatch workerInterrupted = new CountDownLatch(1); @@ -598,9 +134,7 @@ public void interrupt() { assertThat(iterator.hasNext()).isTrue(); assertThat(iterator.next()).isZero(); assertThat(secondStarted.await(3, TimeUnit.SECONDS)).isTrue(); - assertThat(workers.getQueue()).hasSize(1); - assertThat(thirdExecuted).isFalse(); - runQueuedTaskOnInterrupt.set(true); + assertThat(workers.getTaskCount()).isEqualTo(3); Future closeResult = closer.submit( @@ -612,8 +146,6 @@ public void interrupt() { }); assertThat(closeStarted.await(3, TimeUnit.SECONDS)).isTrue(); assertThat(workerInterrupted.await(3, TimeUnit.SECONDS)).isTrue(); - assertThat(interruptHookFinished.await(3, TimeUnit.SECONDS)).isTrue(); - assertThat(runQueuedTaskOnInterrupt).isFalse(); assertThat(closeResult.isDone()).isFalse(); closeThread.get().interrupt(); @@ -637,88 +169,6 @@ public void interrupt() { } } - @Test - public void testCloseDoesNotLeakAnInterruptAfterTaskCompletion() throws Exception { - AtomicBoolean delayInterruptUntilWorkerFinishes = new AtomicBoolean(); - AtomicBoolean workerBlockedBeforeInterrupt = new AtomicBoolean(); - AtomicBoolean interruptedAfterTask = new AtomicBoolean(); - AtomicInteger completedTasks = new AtomicInteger(); - CountDownLatch secondStarted = new CountDownLatch(1); - CountDownLatch releaseSecond = new CountDownLatch(1); - CountDownLatch secondFinished = new CountDownLatch(1); - ThreadPoolExecutor workers = - new ThreadPoolExecutor( - 1, - 1, - 0L, - TimeUnit.MILLISECONDS, - new LinkedBlockingQueue<>(), - runnable -> - new Thread(runnable) { - // close holds BatchTask's monitor here. Release the processor, - // wait for the worker to block publishing FINISHED, then - // deliver the interrupt in the old leak window. - @Override - public void interrupt() { - if (delayInterruptUntilWorkerFinishes.compareAndSet( - true, false)) { - releaseSecond.countDown(); - long deadline = - System.nanoTime() + TimeUnit.SECONDS.toNanos(3); - while (getState() != State.BLOCKED - && System.nanoTime() < deadline) { - Thread.yield(); - } - workerBlockedBeforeInterrupt.set( - getState() == State.BLOCKED); - } - super.interrupt(); - } - }) { - @Override - protected void afterExecute(Runnable runnable, Throwable throwable) { - super.afterExecute(runnable, throwable); - if (completedTasks.incrementAndGet() == 2) { - interruptedAfterTask.set(Thread.currentThread().isInterrupted()); - secondFinished.countDown(); - } - } - }; - ExecutorService closer = Executors.newSingleThreadExecutor(); - CloseableBatchIterator iterator = - ThreadPoolUtils.sequentialBatchedExecuteCloseable( - workers, - input -> { - if (input == 1) { - secondStarted.countDown(); - await(releaseSecond); - } - return Collections.singletonList(input); - }, - Arrays.asList(0, 1), - 2); - - try { - assertThat(iterator.next()).isZero(); - assertThat(secondStarted.await(3, TimeUnit.SECONDS)).isTrue(); - delayInterruptUntilWorkerFinishes.set(true); - - Future closeResult = closer.submit(iterator::close); - assertThat(secondFinished.await(3, TimeUnit.SECONDS)).isTrue(); - closeResult.get(3, TimeUnit.SECONDS); - - assertThat(workerBlockedBeforeInterrupt).isTrue(); - assertThat(interruptedAfterTask).isFalse(); - } finally { - releaseSecond.countDown(); - iterator.close(); - closer.shutdownNow(); - workers.shutdownNow(); - assertThat(closer.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); - assertThat(workers.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); - } - } - @Test public void testClosePreservesPrimaryErrorAndSuppressesWorkerError() throws Exception { ThreadPoolExecutor workers = (ThreadPoolExecutor) Executors.newFixedThreadPool(2); @@ -786,4 +236,28 @@ private static void awaitIgnoringInterrupts(CountDownLatch latch, CountDownLatch } } } + + private static class CountingThreadPoolExecutor extends ThreadPoolExecutor { + + private final AtomicInteger submittedTaskCount = new AtomicInteger(); + + private CountingThreadPoolExecutor(int threadCount) { + super(threadCount, threadCount, 0L, TimeUnit.MILLISECONDS, new LinkedBlockingQueue<>()); + } + + @Override + public void execute(Runnable command) { + submittedTaskCount.incrementAndGet(); + try { + super.execute(command); + } catch (RuntimeException | Error failure) { + submittedTaskCount.decrementAndGet(); + throw failure; + } + } + + private int getSubmittedTaskCount() { + return submittedTaskCount.get(); + } + } } diff --git a/paimon-common/src/test/java/org/apache/paimon/utils/SemaphoredDelegatingExecutorTest.java b/paimon-common/src/test/java/org/apache/paimon/utils/SemaphoredDelegatingExecutorTest.java deleted file mode 100644 index 4dd0282f36aa..000000000000 --- a/paimon-common/src/test/java/org/apache/paimon/utils/SemaphoredDelegatingExecutorTest.java +++ /dev/null @@ -1,112 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.apache.paimon.utils; - -import org.apache.paimon.utils.ThreadPoolUtils.CloseableBatchIterator; - -import org.junit.jupiter.api.Test; - -import java.time.Duration; -import java.util.Arrays; -import java.util.Collections; -import java.util.Iterator; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.Future; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicInteger; - -import static org.apache.paimon.utils.CommonTestUtils.waitUtil; -import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.catchThrowable; - -/** Tests for {@link SemaphoredDelegatingExecutor}. */ -public class SemaphoredDelegatingExecutorTest { - - @Test - public void testFailingTaskDoesNotDeadlockBlockedSubmission() throws Exception { - ExecutorService delegated = Executors.newSingleThreadExecutor(); - SemaphoredDelegatingExecutor workers = - new SemaphoredDelegatingExecutor(delegated, 1, false); - ExecutorService consumer = Executors.newSingleThreadExecutor(); - CountDownLatch firstStarted = new CountDownLatch(1); - CountDownLatch failFirst = new CountDownLatch(1); - AtomicInteger inputsRead = new AtomicInteger(); - RuntimeException workerFailure = new RuntimeException("worker failure"); - Iterator values = Arrays.asList(0, 1, 2).iterator(); - Iterator input = - new Iterator() { - @Override - public boolean hasNext() { - return values.hasNext(); - } - - @Override - public Integer next() { - inputsRead.incrementAndGet(); - return values.next(); - } - }; - CloseableBatchIterator iterator = - ThreadPoolUtils.sequentialSlidingWindowExecuteAwaitRunningTasksOnClose( - workers, - value -> { - if (value == 0) { - firstStarted.countDown(); - await(failFirst); - throw workerFailure; - } - return Collections.singletonList(value); - }, - input, - 2); - Future result = consumer.submit(() -> catchThrowable(() -> iterator.hasNext())); - - try (CloseableBatchIterator ignored = iterator) { - try { - assertThat(firstStarted.await(3, TimeUnit.SECONDS)).isTrue(); - waitUtil( - () -> workers.getWaitingCount() == 1, - Duration.ofSeconds(3), - Duration.ofMillis(10)); - failFirst.countDown(); - - assertThat(result.get(3, TimeUnit.SECONDS)).isSameAs(workerFailure); - assertThat(inputsRead).hasValue(2); - } finally { - failFirst.countDown(); - consumer.shutdownNow(); - assertThat(consumer.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); - } - } finally { - workers.shutdownNow(); - assertThat(workers.awaitTermination(3, TimeUnit.SECONDS)).isTrue(); - } - } - - private static void await(CountDownLatch latch) { - try { - latch.await(); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - throw new RuntimeException(e); - } - } -} diff --git a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java index d46724e29309..76de97dcfb39 100644 --- a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java +++ b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java @@ -51,6 +51,10 @@ import java.io.IOException; import java.io.UncheckedIOException; import java.lang.reflect.Method; +import java.security.AccessControlContext; +import java.security.AccessController; +import java.security.PrivilegedAction; +import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collections; import java.util.HashSet; @@ -60,13 +64,15 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutorService; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Consumer; +import java.util.function.Function; import java.util.stream.Collectors; import static org.apache.paimon.table.format.FormatBatchWriteBuilder.validateStaticPartition; import static org.apache.paimon.utils.ExceptionUtils.firstOrSuppressed; -import static org.apache.paimon.utils.ThreadPoolUtils.CloseableBatchIterator; -import static org.apache.paimon.utils.ThreadPoolUtils.sequentialSlidingWindowExecuteAwaitRunningTasksOnClose; /** Commit for Format Table. */ public class FormatTableCommit implements BatchTableCommit { @@ -119,37 +125,7 @@ public FormatTableCommit( catalogContext, partitionManager, dynamicPartitionOverwrite, - 1); - } - - FormatTableCommit( - String location, - List partitionKeys, - FileIO fileIO, - boolean formatTablePartitionOnlyValueInPath, - String defaultPartName, - boolean overwrite, - Identifier tableIdentifier, - @Nullable Map staticPartitions, - @Nullable String syncHiveUri, - CatalogContext catalogContext, - @Nullable FormatTablePartitionManager partitionManager, - boolean dynamicPartitionOverwrite, - int cleanupThreadNum) { - this( - location, - partitionKeys, - fileIO, - formatTablePartitionOnlyValueInPath, - defaultPartName, - overwrite, - tableIdentifier, - staticPartitions, - syncHiveUri, - catalogContext, - partitionManager, - dynamicPartitionOverwrite, - cleanupThreadNum, + 1, 1); } @@ -400,15 +376,13 @@ private void publishMessages(List messages) throws IOExce return; } - try (CloseableBatchIterator published = - sequentialSlidingWindowExecuteAwaitRunningTasksOnClose( - COMMIT_EXECUTOR, - this::publishMessage, - messages.iterator(), - publishThreadNum)) { - while (published.hasNext()) { - published.next(); - } + try { + executeSideEffects( + COMMIT_EXECUTOR, + this::publishMessage, + messages.iterator(), + publishThreadNum, + ignored -> {}); } catch (UncheckedIOException e) { throw (IOException) unwrapUncheckedIOException(e); } @@ -762,22 +736,253 @@ private Set deletePreviousDataFiles( return clearedPartitionPaths; } // Listing lazily keeps the memory of an overwrite that replaces the table - // proportional to one partition rather than to everything the table holds. Closing - // the iterator is what stops new deletes and waits for the ones already handed out, - // so a failure cannot leave a worker still deleting after this method returns. - try (CloseableBatchIterator cleared = - sequentialSlidingWindowExecuteAwaitRunningTasksOnClose( - COMMIT_EXECUTOR, this::deleteAndReportCleared, dataFiles, threadNum)) { - while (cleared.hasNext()) { - clearedPartitionPaths.add(cleared.next()); - } - } + // proportional to one partition rather than to everything the table holds. The local + // runner stops filling its window and waits for the deletes already handed out, so a + // failure cannot leave a worker still deleting after this method returns. + executeSideEffects( + COMMIT_EXECUTOR, + this::deleteAndReportCleared, + dataFiles, + threadNum, + clearedPartitionPaths::add); } catch (UncheckedIOException e) { throw (IOException) unwrapUncheckedIOException(e); } return clearedPartitionPaths; } + /** + * Runs a bounded sliding window of side effects and consumes their results in input order. + * + *

Once a failure is observed, the runner stops filling the window. Tasks which have not + * started are cancelled, while running tasks are allowed to finish and are drained before the + * failure is returned, so rollback cannot race a side effect already handed to the executor. + */ + private static void executeSideEffects( + ExecutorService executor, + Function> processor, + Iterator input, + int maxConcurrency, + Consumer resultConsumer) { + AtomicBoolean submissionStopped = new AtomicBoolean(); + ArrayDeque> activeTasks = new ArrayDeque<>(maxConcurrency); + long nextInputPosition = 0; + Throwable failure = null; + + try { + while (true) { + while (activeTasks.size() < maxConcurrency && !submissionStopped.get()) { + if (!input.hasNext() || submissionStopped.get()) { + break; + } + I nextInput = input.next(); + if (submissionStopped.get()) { + break; + } + SideEffectTask task = + new SideEffectTask<>( + processor, + nextInput, + nextInputPosition++, + Thread.currentThread().getContextClassLoader(), + AccessController.getContext(), + submissionStopped); + if (submissionStopped.get()) { + break; + } + // Add before execute so a rejected submission is covered by the drain below. + activeTasks.addLast(task); + executor.execute(task); + } + + if (activeTasks.isEmpty()) { + return; + } + + SideEffectTask first = activeTasks.getFirst(); + for (O result : first.result()) { + resultConsumer.accept(result); + } + activeTasks.removeFirst(); + } + } catch (Throwable sideEffectFailure) { + failure = sideEffectFailure; + submissionStopped.set(true); + } + + boolean interrupted = Thread.interrupted(); + for (SideEffectTask task : activeTasks) { + try { + task.cancelIfUnstarted(); + } catch (Throwable cancellationFailure) { + failure = firstOrSuppressed(cancellationFailure, failure); + } + } + // ArrayDeque iteration is input order, so the earliest worker failure is primary unless a + // caller-side listing, submission, consumption, or interruption failure initiated drain. + for (SideEffectTask task : activeTasks) { + while (true) { + try { + task.awaitCompletion(); + break; + } catch (InterruptedException ignored) { + interrupted = true; + } + } + Throwable taskFailure = task.unreportedFailure(); + if (taskFailure != null) { + failure = firstOrSuppressed(taskFailure, failure); + } + } + + if (interrupted) { + Thread.currentThread().interrupt(); + } + throw rethrowSideEffectFailure(failure); + } + + private static RuntimeException rethrowSideEffectFailure(Throwable failure) { + if (failure instanceof Error) { + throw (Error) failure; + } + if (failure instanceof RuntimeException) { + return (RuntimeException) failure; + } + return new RuntimeException(failure); + } + + private static class SideEffectTask implements Runnable { + + private static final int CREATED = 0; + private static final int RUNNING = 1; + private static final int CANCELLED = 2; + private static final int FINISHED = 3; + + private final Function> processor; + private final I input; + private final long inputPosition; + private final ClassLoader callerClassLoader; + private final AccessControlContext callerAccessControlContext; + private final AtomicBoolean submissionStopped; + private final CountDownLatch completion = new CountDownLatch(1); + + private int state = CREATED; + private List result; + private Throwable failure; + private volatile boolean failureReported; + + private SideEffectTask( + Function> processor, + I input, + long inputPosition, + ClassLoader callerClassLoader, + AccessControlContext callerAccessControlContext, + AtomicBoolean submissionStopped) { + this.processor = processor; + this.input = input; + this.inputPosition = inputPosition; + this.callerClassLoader = callerClassLoader; + this.callerAccessControlContext = callerAccessControlContext; + this.submissionStopped = submissionStopped; + } + + @Override + public void run() { + synchronized (this) { + // A queued task which reaches a worker after another task failed has not started + // its side effect and is safe to skip. The volatile stop check is the + // linearization point between a task which was already running and one which can + // still be cancelled. + if (state == CANCELLED || submissionStopped.get()) { + result = Collections.emptyList(); + state = FINISHED; + completion.countDown(); + return; + } + state = RUNNING; + } + + Thread currentThread = Thread.currentThread(); + boolean interruptedOnEntry = currentThread.isInterrupted(); + ClassLoader workerClassLoader = null; + boolean workerClassLoaderCaptured = false; + try { + try { + workerClassLoader = currentThread.getContextClassLoader(); + workerClassLoaderCaptured = true; + currentThread.setContextClassLoader(callerClassLoader); + result = + AccessController.doPrivileged( + (PrivilegedAction>) () -> processor.apply(input), + callerAccessControlContext); + } catch (RuntimeException | Error taskFailure) { + failure = taskFailure; + } finally { + if (workerClassLoaderCaptured) { + try { + currentThread.setContextClassLoader(workerClassLoader); + } catch (RuntimeException | Error restoreFailure) { + failure = firstOrSuppressed(restoreFailure, failure); + } + } + } + if (failure != null) { + submissionStopped.set(true); + } + } finally { + try { + synchronized (this) { + state = FINISHED; + } + // Do not leak an interrupt into a reused worker, while preserving the entry + // state for an executor which runs tasks directly on the caller thread. + Thread.interrupted(); + if (interruptedOnEntry) { + currentThread.interrupt(); + } + } finally { + completion.countDown(); + } + } + } + + private synchronized void cancelIfUnstarted() { + if (state == CREATED) { + state = CANCELLED; + completion.countDown(); + } + } + + private List result() { + if (completion.getCount() != 0) { + try { + completion.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException(e); + } + } + if (failure != null) { + failureReported = true; + throw rethrowSideEffectFailure(failure); + } + return result; + } + + private void awaitCompletion() throws InterruptedException { + completion.await(); + } + + private Throwable unreportedFailure() { + return failureReported ? null : failure; + } + + @Override + public String toString() { + return "FormatTableSideEffectTask{inputPosition=" + inputPosition + '}'; + } + } + /** Unwraps worker I/O failures while preserving recursively suppressed failures. */ private static Throwable unwrapUncheckedIOException(Throwable failure) { if (!(failure instanceof UncheckedIOException)) { diff --git a/paimon-core/src/main/java/org/apache/paimon/utils/ManifestReadThreadPool.java b/paimon-core/src/main/java/org/apache/paimon/utils/ManifestReadThreadPool.java index 7d92195ae8ca..0ef818762d40 100644 --- a/paimon-core/src/main/java/org/apache/paimon/utils/ManifestReadThreadPool.java +++ b/paimon-core/src/main/java/org/apache/paimon/utils/ManifestReadThreadPool.java @@ -61,7 +61,7 @@ public static Iterable sequentialBatchedExecute( executor, processor, input, effectiveThreadNum(threadNum, executor)); } - /** Processes one bounded batch in parallel and waits for it when closed. */ + /** This method parallel processes one bounded batch and waits for it when closed. */ public static ThreadPoolUtils.CloseableBatchIterator sequentialBatchedExecuteCloseable( Function> processor, List input, @Nullable Integer threadNum) { diff --git a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitStatisticsTest.java b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitStatisticsTest.java index 34957fa0caab..5ad2c306e180 100644 --- a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitStatisticsTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitStatisticsTest.java @@ -679,8 +679,7 @@ private FormatTableCommit commit( null, null, partitionManager, - dynamicPartitionOverwrite, - /* cleanupThreadNum */ 1); + dynamicPartitionOverwrite); } /** An overwrite that names no partition: INSERT OVERWRITE without a PARTITION clause. */ diff --git a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java index 4453e6a5f3ad..737fd65adbfa 100644 --- a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java @@ -41,19 +41,29 @@ import org.apache.paimon.utils.PartitionPathUtils; import org.apache.paimon.utils.ReflectionUtils; +import org.apache.paimon.shade.guava30.com.google.common.util.concurrent.MoreExecutors; + import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import org.mockito.ArgumentCaptor; +import javax.security.auth.Subject; + import java.io.FileNotFoundException; import java.io.IOException; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.security.AccessController; +import java.security.PrivilegedAction; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.Comparator; +import java.util.Iterator; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.Callable; import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; @@ -65,6 +75,8 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Consumer; +import java.util.function.Function; import static org.apache.paimon.CoreOptions.PARTITION_DEFAULT_NAME; import static org.apache.paimon.shade.guava30.com.google.common.base.Throwables.getCausalChain; @@ -117,8 +129,7 @@ void testPartitionRegistrationFailureDeletesPublishedTarget() throws Exception { null, null, partitionManager, - /* dynamicPartitionOverwrite */ true, - /* cleanupThreadNum */ 1); + /* dynamicPartitionOverwrite */ true); TwoPhaseCommitMessage message = new TwoPhaseCommitMessage(committer); List messages = Collections.singletonList(message); @@ -169,8 +180,7 @@ void testRegistrationResponseLossStillDeletesPublishedTarget() throws Exception null, null, partitionManager, - /* dynamicPartitionOverwrite */ true, - /* cleanupThreadNum */ 1); + /* dynamicPartitionOverwrite */ true); assertThatThrownBy(() -> commit.commit(Collections.singletonList(message))) .isInstanceOf(RuntimeException.class) @@ -210,8 +220,7 @@ void testHivePostRegistrationFailurePreservesOverwriteTarget() throws Exception null, null, null, - /* dynamicPartitionOverwrite */ true, - /* cleanupThreadNum */ 1); + /* dynamicPartitionOverwrite */ true); PostRegistrationFailingHiveCatalog hiveCatalog = new PostRegistrationFailingHiveCatalog(fileIO, tablePath); ReflectionUtils.setPrivateFieldValue(commit, "hiveCatalog", hiveCatalog); @@ -250,8 +259,7 @@ void testHivePostRegistrationFailureDeletesAppendTarget() throws Exception { null, null, null, - /* dynamicPartitionOverwrite */ true, - /* cleanupThreadNum */ 1); + /* dynamicPartitionOverwrite */ true); PostRegistrationFailingHiveCatalog hiveCatalog = new PostRegistrationFailingHiveCatalog(fileIO, tablePath); ReflectionUtils.setPrivateFieldValue(commit, "hiveCatalog", hiveCatalog); @@ -294,8 +302,7 @@ void testSuccessfulHiveAppendSurvivesAbortAfterMessageRoundTrip() throws Excepti null, null, null, - /* dynamicPartitionOverwrite */ true, - /* cleanupThreadNum */ 1); + /* dynamicPartitionOverwrite */ true); RecordingHiveCatalog hiveCatalog = new RecordingHiveCatalog(fileIO, tablePath); ReflectionUtils.setPrivateFieldValue(commit, "hiveCatalog", hiveCatalog); @@ -315,8 +322,7 @@ void testSuccessfulHiveAppendSurvivesAbortAfterMessageRoundTrip() throws Excepti null, null, null, - /* dynamicPartitionOverwrite */ true, - /* cleanupThreadNum */ 1); + /* dynamicPartitionOverwrite */ true); abortCommit.abort(Collections.singletonList(roundTripped)); assertThat(hiveCatalog.registeredPartitions).containsExactly(staticPartition); @@ -343,8 +349,7 @@ void testFileCommitFailureStillDiscardsUncommittedFiles() throws Exception { null, null, partitionManager, - /* dynamicPartitionOverwrite */ true, - /* cleanupThreadNum */ 1); + /* dynamicPartitionOverwrite */ true); CommitMessage message = new TwoPhaseCommitMessage(committer); assertThatThrownBy(() -> commit.commit(Collections.singletonList(message))) @@ -385,8 +390,7 @@ void testStagingCleanupFailureDeletesPublishedFileBeforeMetadataMutation() throw null, null, partitionManager, - /* dynamicPartitionOverwrite */ true, - /* cleanupThreadNum */ 1); + /* dynamicPartitionOverwrite */ true); assertThatThrownBy( () -> @@ -537,8 +541,7 @@ void testAbortAttemptsEveryRollbackAndReportsDeleteFailure() throws Exception { null, null, null, - /* dynamicPartitionOverwrite */ true, - /* cleanupThreadNum */ 1); + /* dynamicPartitionOverwrite */ true); Throwable failure = catchThrowable(() -> commit.abort(messages)); @@ -636,8 +639,7 @@ void testOverwriteKeepsFilesOfConcurrentWritersStagingTrees() throws Exception { null, null, null, - /* dynamicPartitionOverwrite */ true, - /* cleanupThreadNum */ 1); + /* dynamicPartitionOverwrite */ true); commit.commit(Collections.singletonList(new TwoPhaseCommitMessage(committer))); @@ -686,8 +688,7 @@ void testOverwritingAPrefixKeepsStagingTreesSittingAtAPartitionLevel() throws Ex null, null, null, - /* dynamicPartitionOverwrite */ true, - /* cleanupThreadNum */ 1); + /* dynamicPartitionOverwrite */ true); commit.commit(Collections.emptyList()); @@ -731,8 +732,7 @@ void testOverwritingAPrefixClearsTheDefaultPartitionDirectory() throws Exception null, null, null, - /* dynamicPartitionOverwrite */ true, - /* cleanupThreadNum */ 1); + /* dynamicPartitionOverwrite */ true); commit.commit(Collections.emptyList()); @@ -782,8 +782,7 @@ void testValueOnlyStaticPartitionCannotEscapeTableLocation() throws Exception { null, null, null, - /* dynamicPartitionOverwrite */ true, - /* cleanupThreadNum */ 1); + /* dynamicPartitionOverwrite */ true); assertThatThrownBy(() -> commit.commit(Collections.emptyList())) .isInstanceOf(RuntimeException.class) @@ -1059,6 +1058,262 @@ void testOverwritingTheWholeTableLeavesADirectoryThatIsNoPartitionOfIt() throws assertThat(fileIO.exists(new Path(tablePath, "loose.csv"))).isTrue(); } + @Test + void testLocalSideEffectRunnerPropagatesContextAndRestoresWorkerClassLoader() throws Exception { + ExecutorService workers = Executors.newSingleThreadExecutor(); + try { + ClassLoader workerClassLoader = + workers.submit(() -> Thread.currentThread().getContextClassLoader()) + .get(10, TimeUnit.SECONDS); + ClassLoader callerClassLoader = new ClassLoader(getClass().getClassLoader()) {}; + Subject callerSubject = new Subject(); + AtomicReference seenClassLoader = new AtomicReference<>(); + AtomicReference seenSubject = new AtomicReference<>(); + + ClassLoader originalClassLoader = Thread.currentThread().getContextClassLoader(); + try { + Thread.currentThread().setContextClassLoader(callerClassLoader); + Subject.doAs( + callerSubject, + (PrivilegedAction) + () -> { + try { + invokeSideEffectRunner( + workers, + value -> { + seenClassLoader.set( + Thread.currentThread() + .getContextClassLoader()); + seenSubject.set( + Subject.getSubject( + AccessController.getContext())); + return Collections.singletonList(value); + }, + Collections.singletonList(1).iterator(), + 1, + result -> assertThat(result).isOne()); + return null; + } catch (Exception e) { + throw new RuntimeException(e); + } + }); + } finally { + Thread.currentThread().setContextClassLoader(originalClassLoader); + } + + assertThat(seenClassLoader.get()).isSameAs(callerClassLoader); + assertThat(seenSubject.get()).isSameAs(callerSubject); + assertThat( + workers.submit(() -> Thread.currentThread().getContextClassLoader()) + .get(10, TimeUnit.SECONDS)) + .isSameAs(workerClassLoader); + } finally { + workers.shutdownNow(); + assertThat(workers.awaitTermination(10, TimeUnit.SECONDS)).isTrue(); + } + } + + @Test + void testLocalSideEffectRunnerCancelsAcceptedTaskWhichHasNotStarted() throws Exception { + ExecutorService workers = Executors.newSingleThreadExecutor(); + ExecutorService caller = Executors.newSingleThreadExecutor(); + ExecutorService withholdingExecutor = mock(ExecutorService.class); + CountDownLatch secondTaskAccepted = new CountDownLatch(1); + CountDownLatch releaseFirstTask = new CountDownLatch(1); + AtomicInteger submissions = new AtomicInteger(); + AtomicReference withheldTask = new AtomicReference<>(); + ConcurrentLinkedQueue attemptedInputs = new ConcurrentLinkedQueue<>(); + RuntimeException workerFailure = new RuntimeException("first side effect failed"); + doAnswer( + invocation -> { + Runnable task = invocation.getArgument(0); + if (submissions.getAndIncrement() == 0) { + workers.execute(task); + } else { + withheldTask.set(task); + secondTaskAccepted.countDown(); + } + return null; + }) + .when(withholdingExecutor) + .execute(any(Runnable.class)); + + Future result = + caller.submit( + (Callable) + () -> { + invokeSideEffectRunner( + withholdingExecutor, + input -> { + attemptedInputs.add(input); + if (input == 0) { + try { + if (!releaseFirstTask.await( + 10, TimeUnit.SECONDS)) { + throw new RuntimeException( + "Timed out waiting to release " + + "the first side effect"); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException(e); + } + throw workerFailure; + } + return Collections.emptyList(); + }, + Arrays.asList(0, 1).iterator(), + 2, + ignored -> {}); + return null; + }); + try { + assertThat(secondTaskAccepted.await(10, TimeUnit.SECONDS)).isTrue(); + releaseFirstTask.countDown(); + assertThat(getRootCause(awaitFailure(result))).isSameAs(workerFailure); + Runnable cancelledTask = withheldTask.getAndSet(null); + assertThat(cancelledTask).isNotNull(); + cancelledTask.run(); + assertThat(attemptedInputs).containsExactly(0); + } finally { + releaseFirstTask.countDown(); + Runnable pendingTask = withheldTask.getAndSet(null); + if (pendingTask != null) { + pendingTask.run(); + } + caller.shutdownNow(); + workers.shutdownNow(); + assertThat(caller.awaitTermination(10, TimeUnit.SECONDS)).isTrue(); + assertThat(workers.awaitTermination(10, TimeUnit.SECONDS)).isTrue(); + } + } + + @Test + void testLocalSideEffectRunnerSkipsEarlierInputAfterLaterFailure() throws Exception { + ExecutorService workers = Executors.newSingleThreadExecutor(); + ExecutorService reverseOrderExecutor = mock(ExecutorService.class); + AtomicReference firstTask = new AtomicReference<>(); + ConcurrentLinkedQueue attemptedInputs = new ConcurrentLinkedQueue<>(); + ConcurrentLinkedQueue consumedResults = new ConcurrentLinkedQueue<>(); + RuntimeException workerFailure = new RuntimeException("later side effect failed"); + doAnswer( + invocation -> { + Runnable task = invocation.getArgument(0); + if (firstTask.compareAndSet(null, task)) { + return null; + } + workers.execute( + () -> { + task.run(); + firstTask.get().run(); + }); + return null; + }) + .when(reverseOrderExecutor) + .execute(any(Runnable.class)); + + try { + Throwable failure = + catchThrowable( + () -> + invokeSideEffectRunner( + reverseOrderExecutor, + input -> { + attemptedInputs.add(input); + if (input == 1) { + throw workerFailure; + } + return Collections.singletonList(input); + }, + Arrays.asList(0, 1).iterator(), + 2, + consumedResults::add)); + + assertThat(failure).isSameAs(workerFailure); + assertThat(attemptedInputs).containsExactly(1); + assertThat(consumedResults).isEmpty(); + } finally { + workers.shutdownNow(); + assertThat(workers.awaitTermination(10, TimeUnit.SECONDS)).isTrue(); + } + } + + @Test + void testLocalSideEffectRunnerCompletesWhenContextClassLoaderChangesFail() throws Exception { + SecurityException setFailure = new SecurityException("set TCCL denied"); + SecurityException restoreFailure = new SecurityException("restore TCCL denied"); + AtomicBoolean denyContextClassLoaderChanges = new AtomicBoolean(); + AtomicInteger setAttempts = new AtomicInteger(); + AtomicBoolean processorCalled = new AtomicBoolean(); + ExecutorService workers = + Executors.newSingleThreadExecutor( + runnable -> + new Thread(runnable, "format-table-denied-tccl-worker") { + @Override + public void setContextClassLoader(ClassLoader classLoader) { + if (denyContextClassLoaderChanges.get()) { + throw setAttempts.incrementAndGet() == 1 + ? setFailure + : restoreFailure; + } + super.setContextClassLoader(classLoader); + } + }); + ExecutorService caller = Executors.newSingleThreadExecutor(); + try { + workers.submit(() -> {}).get(10, TimeUnit.SECONDS); + denyContextClassLoaderChanges.set(true); + + Future result = + caller.submit( + () -> + catchThrowable( + () -> + invokeSideEffectRunner( + workers, + input -> { + processorCalled.set(true); + return Collections.singletonList( + input); + }, + Collections.singletonList(1).iterator(), + 1, + ignored -> {}))); + Throwable failure = result.get(10, TimeUnit.SECONDS); + + assertThat(failure).isSameAs(setFailure).hasSuppressedException(restoreFailure); + assertThat(setAttempts).hasValue(2); + assertThat(processorCalled).isFalse(); + } finally { + denyContextClassLoaderChanges.set(false); + caller.shutdownNow(); + workers.shutdownNow(); + assertThat(caller.awaitTermination(10, TimeUnit.SECONDS)).isTrue(); + assertThat(workers.awaitTermination(10, TimeUnit.SECONDS)).isTrue(); + } + } + + @Test + void testLocalSideEffectRunnerPreservesDirectExecutorCallerInterrupt() throws Exception { + ExecutorService directExecutor = MoreExecutors.newDirectExecutorService(); + List consumedResults = new ArrayList<>(); + Thread.currentThread().interrupt(); + try { + invokeSideEffectRunner( + directExecutor, + Collections::singletonList, + Collections.singletonList(1).iterator(), + 1, + consumedResults::add); + + assertThat(consumedResults).containsExactly(1); + assertThat(Thread.currentThread().isInterrupted()).isTrue(); + } finally { + Thread.interrupted(); + directExecutor.shutdownNow(); + } + } + @Test void testCatalogManagedBuilderUses64WayCleanupByDefault() throws Exception { ParallelDeleteFileIO fileIO = new ParallelDeleteFileIO(64, true); @@ -1723,6 +1978,7 @@ void testInterruptDrainsCleanupRestoresFlagAndNeverPublishes() throws Exception assertThat(getCausalChain(failure.get())) .anyMatch(InterruptedException.class::isInstance); assertThat(interruptRestored).isTrue(); + assertThat(fileIO.interruptedDeletes()).isZero(); verify(committer, never()).commit(fileIO); } finally { fileIO.releaseDeletes(); @@ -1755,7 +2011,8 @@ void testCleanupStatisticsClaimOnlyFilesDeletedByThisCommit() throws Exception { null, partitionManager, /* dynamicPartitionOverwrite */ true, - 2); + /* cleanupThreadNum */ 2, + /* publishThreadNum */ 1); commit.commit(Collections.emptyList()); @@ -1836,7 +2093,8 @@ void testConcurrentCleanupReportsCompleteStatisticsAfterBarrier() throws Excepti null, partitionManager, /* dynamicPartitionOverwrite */ true, - 4); + /* cleanupThreadNum */ 4, + /* publishThreadNum */ 1); commit.commit(Collections.emptyList()); @@ -2031,8 +2289,7 @@ private FormatTableCommit overwritingCommit( null, null, null, - dynamicPartitionOverwrite, - /* cleanupThreadNum */ 1); + dynamicPartitionOverwrite); } private FormatTableCommit staticPartitionOverwriteCommit( @@ -2180,7 +2437,8 @@ private FormatTableCommit newCleanupCommit( null, partitionManager, /* dynamicPartitionOverwrite */ true, - cleanupThreadNum); + cleanupThreadNum, + /* publishThreadNum */ 1); } private static void writeOldFiles(LocalFileIO fileIO, Path partitionPath, int count) @@ -2223,6 +2481,36 @@ private static ExecutionException awaitFailure(Future future) throws Exceptio } } + private static void invokeSideEffectRunner( + ExecutorService executor, + Function> processor, + Iterator input, + int maxConcurrency, + Consumer resultConsumer) + throws Exception { + Method method = + FormatTableCommit.class.getDeclaredMethod( + "executeSideEffects", + ExecutorService.class, + Function.class, + Iterator.class, + int.class, + Consumer.class); + method.setAccessible(true); + try { + method.invoke(null, executor, processor, input, maxConcurrency, resultConsumer); + } catch (InvocationTargetException e) { + Throwable cause = e.getCause(); + if (cause instanceof Exception) { + throw (Exception) cause; + } + if (cause instanceof Error) { + throw (Error) cause; + } + throw new RuntimeException(cause); + } + } + private static void awaitLatch(CountDownLatch latch, String description) throws IOException { try { if (!latch.await(10, TimeUnit.SECONDS)) { @@ -2519,6 +2807,7 @@ private static class BlockingDeleteFileIO extends LocalFileIO { private final CountDownLatch deletesStarted; private final CountDownLatch releaseDeletes = new CountDownLatch(1); + private final AtomicInteger interruptedDeletes = new AtomicInteger(); private BlockingDeleteFileIO(int deleteCount) { this.deletesStarted = new CountDownLatch(deleteCount); @@ -2533,6 +2822,7 @@ public boolean delete(Path path, boolean recursive) throws IOException { } return super.delete(path, recursive); } catch (InterruptedException e) { + interruptedDeletes.incrementAndGet(); Thread.currentThread().interrupt(); throw new IOException("Interrupted while blocking cleanup delete", e); } @@ -2545,6 +2835,10 @@ private boolean awaitDeletesStarted() throws InterruptedException { private void releaseDeletes() { releaseDeletes.countDown(); } + + private int interruptedDeletes() { + return interruptedDeletes.get(); + } } private abstract static class SortedLocalFileIO extends LocalFileIO { @@ -2755,8 +3049,7 @@ private FormatTableCommit truncatingCommit( null, null, partitionManager, - /* dynamicPartitionOverwrite */ true, - /* cleanupThreadNum */ 1); + /* dynamicPartitionOverwrite */ true); } private FormatTablePartitionManager commitPartitionedFile( @@ -2781,8 +3074,7 @@ private FormatTablePartitionManager commitPartitionedFile( null, null, partitionManager, - /* dynamicPartitionOverwrite */ true, - /* cleanupThreadNum */ 1); + /* dynamicPartitionOverwrite */ true); commit.commit(Collections.singletonList(new TwoPhaseCommitMessage(committer))); return partitionManager; } diff --git a/paimon-flink/paimon-flink-common/src/main/java/org/apache/paimon/flink/sink/FlinkFormatTableDataStreamSink.java b/paimon-flink/paimon-flink-common/src/main/java/org/apache/paimon/flink/sink/FlinkFormatTableDataStreamSink.java index a223ad51295e..cb32ac54ce31 100644 --- a/paimon-flink/paimon-flink-common/src/main/java/org/apache/paimon/flink/sink/FlinkFormatTableDataStreamSink.java +++ b/paimon-flink/paimon-flink-common/src/main/java/org/apache/paimon/flink/sink/FlinkFormatTableDataStreamSink.java @@ -135,7 +135,13 @@ public void close() throws Exception { } } catch (Exception e) { if (commitMessages != null && !commitMessages.isEmpty()) { - tableCommit.abort(commitMessages); + try { + tableCommit.abort(commitMessages); + } catch (Throwable abortFailure) { + if (abortFailure != e) { + e.addSuppressed(abortFailure); + } + } } throw new RuntimeException(e); } finally { diff --git a/paimon-flink/paimon-flink-common/src/test/java/org/apache/paimon/flink/sink/FlinkFormatTableDataStreamSinkTest.java b/paimon-flink/paimon-flink-common/src/test/java/org/apache/paimon/flink/sink/FlinkFormatTableDataStreamSinkTest.java index 9a838bea6865..c2577ab1f630 100644 --- a/paimon-flink/paimon-flink-common/src/test/java/org/apache/paimon/flink/sink/FlinkFormatTableDataStreamSinkTest.java +++ b/paimon-flink/paimon-flink-common/src/test/java/org/apache/paimon/flink/sink/FlinkFormatTableDataStreamSinkTest.java @@ -24,19 +24,33 @@ import org.apache.paimon.fs.local.LocalFileIO; import org.apache.paimon.options.Options; import org.apache.paimon.table.FormatTable; +import org.apache.paimon.table.format.FormatTableWrite; +import org.apache.paimon.table.sink.BatchTableCommit; +import org.apache.paimon.table.sink.BatchWriteBuilder; +import org.apache.paimon.table.sink.CommitMessage; import org.apache.paimon.types.IntType; import org.apache.paimon.types.RowType; +import org.apache.flink.api.connector.sink2.SinkWriter; import org.apache.flink.streaming.api.lineage.LineageVertex; import org.apache.flink.streaming.api.lineage.LineageVertexProvider; +import org.apache.flink.table.data.RowData; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import java.lang.reflect.Constructor; import java.util.Collections; +import java.util.List; import java.util.Map; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.catchThrowable; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; /** Tests for {@link FlinkFormatTableDataStreamSink}. */ class FlinkFormatTableDataStreamSinkTest { @@ -70,4 +84,47 @@ void testFormatTableSinkLineageVertex() throws Exception { assertThat(vertex.datasets()).hasSize(1); assertThat(vertex.datasets().get(0).name()).isEqualTo("paimon." + table.fullName()); } + + @Test + void testClosePreservesCommitFailureWhenSecondAbortFails() throws Exception { + FormatTable table = mock(FormatTable.class); + BatchWriteBuilder writeBuilder = mock(BatchWriteBuilder.class); + FormatTableWrite tableWrite = mock(FormatTableWrite.class); + BatchTableCommit tableCommit = mock(BatchTableCommit.class); + CommitMessage message = mock(CommitMessage.class); + List messages = Collections.singletonList(message); + RuntimeException commitFailure = new RuntimeException("publish response lost"); + RuntimeException abortFailure = new RuntimeException("multipart upload no longer exists"); + when(table.newBatchWriteBuilder()).thenReturn(writeBuilder); + when(writeBuilder.newWrite()).thenReturn(tableWrite); + when(writeBuilder.newCommit()).thenReturn(tableCommit); + when(tableWrite.prepareCommit()).thenReturn(messages); + doNothing().doThrow(abortFailure).when(tableCommit).abort(messages); + doAnswer( + invocation -> { + tableCommit.abort(messages); + throw commitFailure; + }) + .when(tableCommit) + .commit(messages); + + Class writerClass = + Class.forName( + "org.apache.paimon.flink.sink.FlinkFormatTableDataStreamSink$" + + "FormatTableSink$FormatTableSinkWriter"); + Constructor constructor = + writerClass.getDeclaredConstructor(FormatTable.class, boolean.class, Map.class); + constructor.setAccessible(true); + @SuppressWarnings("unchecked") + SinkWriter writer = + (SinkWriter) constructor.newInstance(table, false, Collections.emptyMap()); + + Throwable failure = catchThrowable(writer::close); + + assertThat(failure).isInstanceOf(RuntimeException.class); + assertThat(failure.getCause()).isSameAs(commitFailure); + assertThat(commitFailure.getSuppressed()).containsExactly(abortFailure); + verify(tableCommit, times(2)).abort(messages); + verify(tableWrite).close(); + } }