This is an automated email from the ASF dual-hosted git repository.
jshao pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 7b94ecadc1 [#7221] Fix exception handling: catch Exception instead of
Throwable where appropriate (#7229)
7b94ecadc1 is described below
commit 7b94ecadc1fee2de623b8645fba11e21e43a4f79
Author: Guilherme Santos <[email protected]>
AuthorDate: Mon May 26 02:56:25 2025 +0100
[#7221] Fix exception handling: catch Exception instead of Throwable where
appropriate (#7229)
### What changes were proposed in this pull request?
Fix #7221
### Why are the changes needed?
Several files catch `Throwable` when they should catch `Exception`. This
is problematic because catching `Throwable` also catches serious JVM
errors like `OutOfMemoryError`.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
All build tests passed.
---
.../gravitino/catalog/OperationDispatcher.java | 40 +++++++++++-----------
.../storage/relational/utils/SessionUtils.java | 16 ++++-----
.../connector/store/GravitinoCatalogStore.java | 11 +++---
.../connector/integration/test/FlinkEnvIT.java | 4 +--
.../integration/test/sql/SQLQueryTestHelper.java | 2 +-
.../connector/GravitinoConnectorPluginManager.java | 4 +--
6 files changed, 37 insertions(+), 40 deletions(-)
diff --git
a/core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java
b/core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java
index 887468edc7..457aaee493 100644
--- a/core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java
+++ b/core/src/main/java/org/apache/gravitino/catalog/OperationDispatcher.java
@@ -79,14 +79,14 @@ public abstract class OperationDispatcher {
NameIdentifier catalogIdent = getCatalogIdentifier(tableIdent);
CatalogManager.CatalogWrapper c =
catalogManager.loadCatalogAndWrap(catalogIdent);
return c.doWithPartitionOps(tableIdent, fn);
- } catch (Throwable throwable) {
- if (ex.isInstance(throwable)) {
- throw ex.cast(throwable);
+ } catch (Exception exception) {
+ if (ex.isInstance(exception)) {
+ throw ex.cast(exception);
}
- if (RuntimeException.class.isAssignableFrom(throwable.getClass())) {
- throw (RuntimeException) throwable;
+ if (RuntimeException.class.isAssignableFrom(exception.getClass())) {
+ throw (RuntimeException) exception;
}
- throw new RuntimeException(throwable);
+ throw new RuntimeException(exception);
}
}
@@ -98,14 +98,14 @@ public abstract class OperationDispatcher {
try {
CatalogManager.CatalogWrapper c =
catalogManager.loadCatalogAndWrap(ident);
return fn.apply(c);
- } catch (Throwable throwable) {
- if (ex.isInstance(throwable)) {
- throw ex.cast(throwable);
+ } catch (Exception exception) {
+ if (ex.isInstance(exception)) {
+ throw ex.cast(exception);
}
- if (RuntimeException.class.isAssignableFrom(throwable.getClass())) {
- throw (RuntimeException) throwable;
+ if (RuntimeException.class.isAssignableFrom(exception.getClass())) {
+ throw (RuntimeException) exception;
}
- throw new RuntimeException(throwable);
+ throw new RuntimeException(exception);
}
}
@@ -120,17 +120,17 @@ public abstract class OperationDispatcher {
try {
CatalogManager.CatalogWrapper c =
catalogManager.loadCatalogAndWrap(ident);
return fn.apply(c);
- } catch (Throwable throwable) {
- if (ex1.isInstance(throwable)) {
- throw ex1.cast(throwable);
- } else if (ex2.isInstance(throwable)) {
- throw ex2.cast(throwable);
+ } catch (Exception exception) {
+ if (ex1.isInstance(exception)) {
+ throw ex1.cast(exception);
+ } else if (ex2.isInstance(exception)) {
+ throw ex2.cast(exception);
}
- if (RuntimeException.class.isAssignableFrom(throwable.getClass())) {
- throw (RuntimeException) throwable;
+ if (RuntimeException.class.isAssignableFrom(exception.getClass())) {
+ throw (RuntimeException) exception;
}
- throw new RuntimeException(throwable);
+ throw new RuntimeException(exception);
}
}
diff --git
a/core/src/main/java/org/apache/gravitino/storage/relational/utils/SessionUtils.java
b/core/src/main/java/org/apache/gravitino/storage/relational/utils/SessionUtils.java
index 8bc18ca923..5138b4dcbb 100644
---
a/core/src/main/java/org/apache/gravitino/storage/relational/utils/SessionUtils.java
+++
b/core/src/main/java/org/apache/gravitino/storage/relational/utils/SessionUtils.java
@@ -46,9 +46,9 @@ public class SessionUtils {
T mapper = SqlSessions.getMapper(mapperClazz);
consumer.accept(mapper);
SqlSessions.commitAndCloseSqlSession();
- } catch (Throwable t) {
+ } catch (Exception e) {
SqlSessions.rollbackAndCloseSqlSession();
- throw t;
+ throw e;
}
}
}
@@ -70,9 +70,9 @@ public class SessionUtils {
R result = func.apply(mapper);
SqlSessions.commitAndCloseSqlSession();
return result;
- } catch (Throwable t) {
+ } catch (Exception e) {
SqlSessions.rollbackAndCloseSqlSession();
- throw t;
+ throw e;
}
}
}
@@ -120,8 +120,8 @@ public class SessionUtils {
try {
T mapper = SqlSessions.getMapper(mapperClazz);
return func.apply(mapper);
- } catch (Throwable t) {
- throw t;
+ } catch (Exception e) {
+ throw e;
} finally {
SqlSessions.closeSqlSession();
}
@@ -139,9 +139,9 @@ public class SessionUtils {
try {
Arrays.stream(operations).forEach(Runnable::run);
SqlSessions.commitAndCloseSqlSession();
- } catch (Throwable t) {
+ } catch (Exception e) {
SqlSessions.rollbackAndCloseSqlSession();
- throw t;
+ throw e;
}
}
}
diff --git
a/flink-connector/flink/src/main/java/org/apache/gravitino/flink/connector/store/GravitinoCatalogStore.java
b/flink-connector/flink/src/main/java/org/apache/gravitino/flink/connector/store/GravitinoCatalogStore.java
index 836f0fc056..726cf6aa6b 100644
---
a/flink-connector/flink/src/main/java/org/apache/gravitino/flink/connector/store/GravitinoCatalogStore.java
+++
b/flink-connector/flink/src/main/java/org/apache/gravitino/flink/connector/store/GravitinoCatalogStore.java
@@ -152,13 +152,10 @@ public class GravitinoCatalogStore extends
AbstractCatalogStore {
if (catalogFactory instanceof BaseCatalogFactory &&
predicate.test(catalogFactory)) {
factories.add(catalogFactory);
}
- } catch (Throwable t) {
- if (t instanceof NoClassDefFoundError) {
- LOG.debug(
- "NoClassDefFoundError when loading a " +
Factory.class.getCanonicalName() + ".", t);
- } else {
- throw new RuntimeException("Unexpected error when trying to load
service provider.", t);
- }
+ } catch (NoClassDefFoundError e) {
+ LOG.debug("NoClassDefFoundError when loading a {}.",
Factory.class.getCanonicalName(), e);
+ } catch (Exception e) {
+ throw new RuntimeException("Unexpected error when trying to load
service provider.", e);
}
}
diff --git
a/flink-connector/flink/src/test/java/org/apache/gravitino/flink/connector/integration/test/FlinkEnvIT.java
b/flink-connector/flink/src/test/java/org/apache/gravitino/flink/connector/integration/test/FlinkEnvIT.java
index 1a37cc7f4b..e2cdbbd69b 100644
---
a/flink-connector/flink/src/test/java/org/apache/gravitino/flink/connector/integration/test/FlinkEnvIT.java
+++
b/flink-connector/flink/src/test/java/org/apache/gravitino/flink/connector/integration/test/FlinkEnvIT.java
@@ -203,8 +203,8 @@ public abstract class FlinkEnvIT extends BaseIT {
try {
TableEnvironmentImpl env = (TableEnvironmentImpl) tableEnv;
env.getCatalogManager().close();
- } catch (Throwable throwable) {
- LOG.error("Close Flink environment failed", throwable);
+ } catch (Exception e) {
+ LOG.error("Close Flink environment failed", e);
}
}
}
diff --git
a/spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/integration/test/sql/SQLQueryTestHelper.java
b/spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/integration/test/sql/SQLQueryTestHelper.java
index 39c29a4a08..c2424f57a2 100644
---
a/spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/integration/test/sql/SQLQueryTestHelper.java
+++
b/spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/integration/test/sql/SQLQueryTestHelper.java
@@ -82,7 +82,7 @@ public class SQLQueryTestHelper {
Supplier<Pair<String, List<String>>> result) {
try {
return result.get();
- } catch (Throwable e) {
+ } catch (Exception e) {
return Pair.of(emptySchema, Arrays.asList("[SPARK_EXCEPTION]"));
}
}
diff --git
a/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnectorPluginManager.java
b/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnectorPluginManager.java
index 51bd5f0948..f343d3a4c9 100644
---
a/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnectorPluginManager.java
+++
b/trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnectorPluginManager.java
@@ -219,8 +219,8 @@ public class GravitinoConnectorPluginManager {
String key = v.substring(start, end).replace("trino-", "");
try {
loadPluginByPom(artifactResolver.resolvePom(new File(v)), key);
- } catch (Throwable t) {
- LOG.error("Fatal error in load plugin by {}", v, t);
+ } catch (Exception e) {
+ LOG.error("Fatal error in load plugin by {}", v, e);
}
});
}
