This is an automated email from the ASF dual-hosted git repository.
gortiz pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 9de73ec47cb Refactor exception handling to use ExceptionUtils for
better suppression of multiple exceptions (#17724)
9de73ec47cb is described below
commit 9de73ec47cb50ffbc09efa6d454a9db7eb0abdc2
Author: Gonzalo Ortiz Jaureguizar <[email protected]>
AuthorDate: Fri Feb 20 08:44:46 2026 +0100
Refactor exception handling to use ExceptionUtils for better suppression of
multiple exceptions (#17724)
* Refactor exception handling to use ExceptionUtils for better suppression
of multiple exceptions
* Fix checkstyle issues
---
.../apache/pinot/common/utils/ExceptionUtils.java | 109 +++++++++++++++++++++
.../org/apache/pinot/common/utils/FileUtils.java | 6 +-
.../core/data/manager/BaseTableDataManager.java | 3 +-
.../runtime/operator/exchange/BlockExchange.java | 17 +---
.../impl/SegmentIndexCreationDriverImpl.java | 3 +-
.../segment/spi/memory/PagedPinotOutputStream.java | 3 +-
6 files changed, 119 insertions(+), 22 deletions(-)
diff --git
a/pinot-common/src/main/java/org/apache/pinot/common/utils/ExceptionUtils.java
b/pinot-common/src/main/java/org/apache/pinot/common/utils/ExceptionUtils.java
index 010ae4cc7f3..37da64b14ee 100644
---
a/pinot-common/src/main/java/org/apache/pinot/common/utils/ExceptionUtils.java
+++
b/pinot-common/src/main/java/org/apache/pinot/common/utils/ExceptionUtils.java
@@ -18,6 +18,8 @@
*/
package org.apache.pinot.common.utils;
+import java.util.function.Function;
+import javax.annotation.Nullable;
import org.apache.commons.lang3.StringUtils;
@@ -83,4 +85,111 @@ public class ExceptionUtils {
}
return sb.toString();
}
+
+ /// Suppress the new throwable by adding it as suppressed to the old
throwable if the old throwable is not null.
+ /// Otherwise, return the new throwable.
+ ///
+ /// This is usually used in cases we want to call a function on all elements
of a collection even if some of the calls
+ /// throw exception, but we don't want to lose the exception information
from other calls.
+ /// For example, when closing multiple resources, we want to close all
resources even if some of them throw exception,
+ /// and we want to keep the exception information from all resources instead
of just the first one.
+ ///
+ /// Use this function when the new throwable and old throwable are of the
same type, so that we can directly add the
+ /// new throwable as suppressed to the old throwable without needing to
convert the exception type.
+ /// Use [suppress(T2, T1, Function, Class)](ExceptionUtils#supress(T2, T1,
Function, Class)) when the new throwable
+ /// and old throwable are of different types, and we need to convert the
exception type before adding as suppressed.
+ ///
+ /// Normal usage is something like:
+ ///
+ /// ```java
+ /// MyException throwable = null;
+ /// for (Resource resource : resources) {
+ /// try {
+ /// resource.close();
+ /// } catch (MyException e) {
+ /// throwable = ExceptionUtils.suppress(e, throwable);
+ /// }
+ /// }
+ /// if (throwable != null) {
+ /// throw throwable;
+ /// }
+ /// ```
+ public static <T extends Throwable> T suppress(
+ T newThrowable,
+ @Nullable T oldThrowable
+ ) {
+ if (oldThrowable != null) {
+ if (oldThrowable != newThrowable) {
+ // We only add the new throwable as suppressed to the old throwable
when they are different.
+ // Otherwise, it will cause infinite loop when printing stack trace.
+ // This is pretty common when -XX:+OmitStackTraceInFastThrow is
enabled, which will reuse the same throwable
+ // instance for the same exception type.
+ oldThrowable.addSuppressed(newThrowable);
+ }
+ return oldThrowable;
+ }
+ return newThrowable;
+ }
+
+ /// Suppress the new throwable by adding it as suppressed to the old
throwable if the old throwable is not null.
+ /// Otherwise, return the new throwable.
+ ///
+ /// This is usually used in cases we want to call a function on all elements
of a collection even if some of the calls
+ /// throw exception, but we don't want to lose the exception information
from other calls.
+ /// For example, when closing multiple resources, we want to close all
resources even if some of them throw exception,
+ /// and we want to keep the exception information from all resources instead
of just the first one.
+ ///
+ /// Use this function when the new throwable and old throwable are of
different types, and we need to convert the
+ /// exception type before adding as suppressed. Use [suppress(T,
T)](ExceptionUtils#supress(T, T)) when the new
+ /// throwable and old throwable are of the same type, so that we can
directly add the new throwable as suppressed to
+ /// the old throwable.
+ ///
+ /// Normal usage is something like:
+ ///
+ /// ```java
+ /// RuntimeException throwable = null;
+ /// for (Resource resource : resources) {
+ /// try {
+ /// resource.close();
+ /// } catch (Exception e) {
+ /// throwable = ExceptionUtils.supress(e, throwable,
RuntimeException::new);
+ /// }
+ /// }
+ /// if (throwable != null) {
+ /// throw throwable;
+ /// }
+ /// ```
+ ///
+ /// @param newThrowable the new throwable to be suppressed
+ /// @param oldThrowable the old throwable to be added with the new throwable
as suppressed,
+ /// or null if there is no old throwable
+ /// @param exceptionConverter the function to convert the new throwable to
the same type as the old throwable,
+ /// which will be used when the old throwable is not null
+ /// @param clazz the class of the old throwable. If old throwable is null
and this parameter is provided,
+ /// we will check if the new throwable is already of the
same type as the old throwable,
+ /// and if so, we can directly return the new throwable
without conversion.
+ public static <TO extends Throwable, TI extends Throwable> TO suppress(
+ TI newThrowable,
+ @Nullable TO oldThrowable,
+ Function<TI, TO> exceptionConverter,
+ @Nullable Class<TO> clazz
+ ) {
+ if (oldThrowable != null) {
+ if (oldThrowable != newThrowable) {
+ // We only add the new throwable as suppressed to the old throwable
when they are different.
+ // Otherwise, it will cause infinite loop when printing stack trace.
+ // This is pretty common when -XX:+OmitStackTraceInFastThrow is
enabled, which will reuse the same throwable
+ // instance for the same exception type.
+ oldThrowable.addSuppressed(newThrowable);
+ }
+ return oldThrowable;
+ }
+ if (clazz != null && clazz.isAssignableFrom(newThrowable.getClass())) {
+ // If the new throwable is already of the same type as the old
throwable, we can directly return it without
+ // conversion. This is an optimization to avoid unnecessary exception
conversion when the new throwable is already
+ // of the same type.
+ return (TO) newThrowable;
+ }
+ return exceptionConverter.apply(newThrowable);
+ }
}
diff --git
a/pinot-common/src/main/java/org/apache/pinot/common/utils/FileUtils.java
b/pinot-common/src/main/java/org/apache/pinot/common/utils/FileUtils.java
index 62df4341283..41fdd02e236 100644
--- a/pinot-common/src/main/java/org/apache/pinot/common/utils/FileUtils.java
+++ b/pinot-common/src/main/java/org/apache/pinot/common/utils/FileUtils.java
@@ -92,11 +92,7 @@ public class FileUtils {
closeable.close();
}
} catch (IOException e) {
- if (topLevelException == null) {
- topLevelException = e;
- } else if (e != topLevelException) {
- topLevelException.addSuppressed(e);
- }
+ topLevelException = ExceptionUtils.suppress(e, topLevelException);
}
}
diff --git
a/pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java
b/pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java
index 5694a9aaecc..df1ddb2fbea 100644
---
a/pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java
+++
b/pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java
@@ -59,6 +59,7 @@ import org.apache.pinot.common.metrics.ServerMeter;
import org.apache.pinot.common.metrics.ServerMetrics;
import org.apache.pinot.common.metrics.ServerTimer;
import org.apache.pinot.common.restlet.resources.SegmentErrorInfo;
+import org.apache.pinot.common.utils.ExceptionUtils;
import org.apache.pinot.common.utils.TarCompressionUtils;
import org.apache.pinot.common.utils.config.TierConfigUtils;
import org.apache.pinot.common.utils.fetcher.SegmentFetcherFactory;
@@ -923,7 +924,7 @@ public abstract class BaseTableDataManager implements
TableDataManager {
LoaderUtils.reloadFailureRecovery(indexDir);
} catch (Exception recoveryFailureException) {
_logger.error("Failed to recover segment: {} after reload failure",
segmentName, recoveryFailureException);
- reloadFailureException.addSuppressed(recoveryFailureException);
+ ExceptionUtils.suppress(recoveryFailureException,
reloadFailureException);
}
addSegmentError(segmentName,
new SegmentErrorInfo(System.currentTimeMillis(), "Caught exception
while reloading segment",
diff --git
a/pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/exchange/BlockExchange.java
b/pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/exchange/BlockExchange.java
index 02292b180e6..2cc5efb2114 100644
---
a/pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/exchange/BlockExchange.java
+++
b/pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/exchange/BlockExchange.java
@@ -23,6 +23,7 @@ import java.util.List;
import java.util.concurrent.ThreadLocalRandom;
import java.util.function.Function;
import org.apache.calcite.rel.RelDistribution;
+import org.apache.pinot.common.utils.ExceptionUtils;
import org.apache.pinot.query.mailbox.ReceivingMailbox;
import org.apache.pinot.query.mailbox.SendingMailbox;
import org.apache.pinot.query.planner.partitioning.KeySelectorFactory;
@@ -143,11 +144,7 @@ public abstract class BlockExchange implements
AutoCloseable {
}
} catch (RuntimeException e) {
// We want to try to send EOS to all mailboxes, so we catch the
exception and rethrow it at the end.
- if (firstException == null) {
- firstException = e;
- } else {
- firstException.addSuppressed(e);
- }
+ firstException = ExceptionUtils.suppress(e, firstException);
}
}
if (firstException != null) {
@@ -183,15 +180,7 @@ public abstract class BlockExchange implements
AutoCloseable {
try {
sendingMailbox.close();
} catch (Exception e) {
- if (firstException == null) {
- if (firstException instanceof RuntimeException) {
- firstException = (RuntimeException) e;
- } else {
- firstException = new RuntimeException(e);
- }
- } else {
- firstException.addSuppressed(e);
- }
+ firstException = ExceptionUtils.suppress(e, firstException,
RuntimeException::new, RuntimeException.class);
}
}
if (firstException != null) {
diff --git
a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentIndexCreationDriverImpl.java
b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentIndexCreationDriverImpl.java
index c0c10b3d30a..5a8770c2f06 100644
---
a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentIndexCreationDriverImpl.java
+++
b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentIndexCreationDriverImpl.java
@@ -32,6 +32,7 @@ import org.apache.pinot.common.metrics.MinionMeter;
import org.apache.pinot.common.metrics.MinionMetrics;
import org.apache.pinot.common.metrics.ServerMeter;
import org.apache.pinot.common.metrics.ServerMetrics;
+import org.apache.pinot.common.utils.ExceptionUtils;
import
org.apache.pinot.segment.local.realtime.converter.stats.RealtimeSegmentSegmentCreationDataSource;
import
org.apache.pinot.segment.local.segment.creator.ColumnarSegmentCreationDataSource;
import
org.apache.pinot.segment.local.segment.creator.RecordReaderSegmentCreationDataSource;
@@ -641,7 +642,7 @@ public class SegmentIndexCreationDriverImpl implements
SegmentIndexCreationDrive
} catch (Exception closeException) {
LOGGER.error("Error closing index creator", closeException);
// Add the close exception as suppressed to preserve both exceptions
- e.addSuppressed(closeException);
+ ExceptionUtils.suppress(closeException, e);
}
throw e;
} finally {
diff --git
a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/memory/PagedPinotOutputStream.java
b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/memory/PagedPinotOutputStream.java
index 1e54c6fa067..db2599b8045 100644
---
a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/memory/PagedPinotOutputStream.java
+++
b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/memory/PagedPinotOutputStream.java
@@ -277,9 +277,10 @@ public class PagedPinotOutputStream extends
PinotOutputStream {
try {
_allocator.release(page);
} catch (IOException e) {
+ // This module doesn't import pinot-common, so we cannot use
ExceptionUtils.suppress here.
if (ex == null) {
ex = e;
- } else {
+ } else if (e != ex) {
ex.addSuppressed(e);
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
