jerryshao commented on code in PR #13787:
URL: https://github.com/apache/gravitino/pull/13787#discussion_r4255236739
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoErrorCode.java:
##########
@@ -94,15 +102,59 @@ public ErrorCode toErrorCode() {
}
/**
- * Converts an exception's message to a simplified error message. If the
message contains multiple
- * lines, returns the first two lines concatenated. If the message contains
only one line, returns
- * that line.
+ * Converts an exception into a Trino exception. See {@link
#toTrinoException(Exception, String)}.
*
- * @param e the exception whose message should be simplified
- * @return the simplified error message
+ * @param e the exception to convert
+ * @return the Trino exception to throw
*/
- public static String toSimpleErrorMessage(Exception e) {
- List<String> lines = e.getMessage().lines().toList();
- return lines.size() > 1 ? lines.get(0) + lines.get(1) : lines.get(0);
+ public static TrinoException toTrinoException(Exception e) {
+ return toTrinoException(e, null);
+ }
+
+ /**
+ * Converts an exception into a Trino exception. A {@link TrinoException} is
returned as is, and
+ * an {@link ExecutionException} is classified by its cause. {@link
ForbiddenException}, {@link
+ * NotInUseException}, {@link IllegalArgumentException} and {@link
UnsupportedOperationException}
+ * map to {@code PERMISSION_DENIED}, {@code CATALOG_UNAVAILABLE}, {@link
+ * #GRAVITINO_ILLEGAL_ARGUMENT} and {@link
#GRAVITINO_UNSUPPORTED_OPERATION}. Anything else maps
+ * to {@code GENERIC_INTERNAL_ERROR}.
+ *
+ * @param exception the exception to convert
+ * @param context describes the failed operation; it prefixes the message of
an exception that
+ * maps to {@code GENERIC_INTERNAL_ERROR}, whose own message may not say
what failed
+ * @return the Trino exception to throw
+ */
+ public static TrinoException toTrinoException(Exception exception, @Nullable
String context) {
+ Exception e =
+ exception instanceof ExecutionException && exception.getCause()
instanceof Exception
+ ? (Exception) exception.getCause()
+ : exception;
+ if (e instanceof TrinoException) {
+ return (TrinoException) e;
+ }
+ if (e instanceof ForbiddenException) {
+ return new TrinoException(PERMISSION_DENIED, firstLineMessage(e), e);
+ }
+ if (e instanceof NotInUseException) {
+ return new TrinoException(CATALOG_UNAVAILABLE, firstLineMessage(e), e);
+ }
+ if (e instanceof IllegalArgumentException) {
+ return new TrinoException(GRAVITINO_ILLEGAL_ARGUMENT,
firstLineMessage(e), e);
+ }
+ if (e instanceof UnsupportedOperationException) {
+ return new TrinoException(GRAVITINO_UNSUPPORTED_OPERATION,
firstLineMessage(e), e);
+ }
+ String message = firstLineMessage(e);
+ return new TrinoException(
+ GENERIC_INTERNAL_ERROR, context == null ? message : context + ": " +
message, e);
Review Comment:
[Question] Should an unreachable or unclassifiable server land here, or on
`CATALOG_UNAVAILABLE`?
`RestErrorHandler.accept`, the base every handler falls back to, throws
`ConnectionFailedException` for CONNECTION_FAILED_CODE
(clients/client-java/src/main/java/org/apache/gravitino/client/ErrorHandlers.java:1430-1432)
and `RESTException` for any code it cannot classify (:1436). Neither is a
Trino-internal fault - the Gravitino server is down or answering something
unexpected - yet both land on this line as INTERNAL_ERROR, which is the 'looks
like a server fault' symptom #13782 names, and in practice they are far more
common than a refusal. `CATALOG_UNAVAILABLE` is already imported two branches
up and fits them.
Not a defect in what the PR claims to do, so happy to leave it; if it is a
deliberate follow-up, a line in the description would save the next reader the
derivation.
Verified by: reading ErrorHandlers.java:1410-1437 and
GravitinoErrorCode.java:122-150 in this checkout.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorMetadata.java:
##########
@@ -212,6 +229,8 @@ public void createSchema(GravitinoSchema schema) {
} catch (TableAlreadyExistsException e) {
throw new TrinoException(
GravitinoErrorCode.GRAVITINO_SCHEMA_ALREADY_EXISTS, "Schema already
exists", e);
+ } catch (RuntimeException e) {
+ throw GravitinoErrorCode.toTrinoException(e);
Review Comment:
[Important] A duplicate `CREATE SCHEMA` still reaches the user as
GENERIC_INTERNAL_ERROR, because the catch just above this one can never fire.
`SupportsSchemas.createSchema` declares `throws NoSuchCatalogException,
SchemaAlreadyExistsException`
(api/src/main/java/org/apache/gravitino/SupportsSchemas.java:108-109), and over
REST the client turns ALREADY_EXISTS_CODE into `SchemaAlreadyExistsException`
(clients/client-java/src/main/java/org/apache/gravitino/client/ErrorHandlers.java:525-526,
in `SchemaErrorHandler` at :505). `SchemaAlreadyExistsException` and
`TableAlreadyExistsException` are siblings - both extend
`AlreadyExistsException`, neither extends the other - so the `catch
(TableAlreadyExistsException)` on line 229 is unreachable on this path and the
refusal drops into this new generic catch, which classifies it as
GENERIC_INTERNAL_ERROR. The `catch (NoSuchSchemaException)` on line 226 has the
same problem: `createSchema` raises `NoSuchCatalogException`, so a missing
catalog also lands here.
The mis-wiring predates the PR - before it, the same exception escaped to
the engine and was reported as an internal error too - but this is now the
catch that absorbs it, and classifying expected refusals is precisely what this
PR is for. The fix is one type name each: catch `SchemaAlreadyExistsException`
for GRAVITINO_SCHEMA_ALREADY_EXISTS and `NoSuchCatalogException` for
GRAVITINO_CATALOG_NOT_EXISTS. A `createSchema` test would also close part of
the coverage gap noted in the body.
Verified by: reading SupportsSchemas.java:99-140, the `SchemaErrorHandler`
block of ErrorHandlers.java:505-556, and SchemaAlreadyExistsException.java:25 /
TableAlreadyExistsException.java:25 in this checkout.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoConnector.java:
##########
@@ -291,11 +291,7 @@ CatalogConnectorMetadata
resolveSessionMetadata(ConnectorSession session) {
Throwable cause = e.getCause() == null ? e : e.getCause();
LOG.warn(
cause, "Failed to create per-user Gravitino client for user '%s'",
session.getUser());
- if (cause instanceof TrinoException) {
- // Already carries a specific Trino error code (e.g. from
buildForSession); re-wrapping
- // would swallow it.
- throw (TrinoException) cause;
- }
+ // buildAuthClient raises these for an invalid or unsupported credential.
Review Comment:
[Question] Is this comment the whole story - can the branch below also catch
an `IllegalArgumentException` that has nothing to do with the credential?
The cache loader on lines 280-288 calls `buildAuthClient(session)` and then
`userClient.loadMetalake(...)`, and `MetalakeErrorHandler` maps
ILLEGAL_ARGUMENTS_CODE to a plain `IllegalArgumentException`
(clients/client-java/src/main/java/org/apache/gravitino/client/ErrorHandlers.java:639-640,
handler at :631). So a server-side argument rejection from `loadMetalake` is
reported as PERMISSION_DENIED with 'Failed to authenticate user 'x' with
Gravitino: ...' even though authentication succeeded - the same class of
mislabel #13782 is about, pointing the other way. The `new
CatalogConnectorMetadata(...)` on the following line is safe, since it converts
`IllegalArgumentException` into a `TrinoException` itself and the passthrough
preserves it.
Pre-existing, and the branch is right for what the comment describes. Worth
narrowing it to the `buildAuthClient` call (or letting `toTrinoException`
classify the rest) so the message stays honest? If it is deliberate as is, no
change needed.
Verified by: reading GravitinoConnector.java:265-318 and the
`MetalakeErrorHandler` block of ErrorHandlers.java:631-662 in this checkout;
`RestErrorHandler.accept` (:1425-1436) is what makes the new
`testResolveSessionMetadataMapsLoadMetalakeDenialToPermissionDenied` realistic,
since `MetalakeErrorHandler` itself has no FORBIDDEN_CODE case.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/CatalogConnectorMetadata.java:
##########
@@ -233,6 +252,8 @@ public void dropSchema(String schemaName, boolean cascade) {
} catch (NonEmptySchemaException e) {
throw new TrinoException(
GravitinoErrorCode.GRAVITINO_SCHEMA_NOT_EMPTY, "Schema does not
empty", e);
+ } catch (RuntimeException e) {
+ throw GravitinoErrorCode.toTrinoException(e);
Review Comment:
[Nit] This catch also catches the `TrinoException` thrown inside its own try
block at lines 247-250 ('Drop schema failed'), and the result is correct only
because `toTrinoException` returns a `TrinoException` unchanged.
Harmless today, but it makes the error code of a hand-thrown failure depend
on a passthrough several files away. `dropTable` (lines 266-276) already solves
this the clean way by moving the `if (!dropped)` check out of the try; the same
shape would work here. The three stored procedures lean on the same
passthrough, deliberately - `DropCatalogStoredProcedure.java:106-109` and the
updated `00005_create_catalog.txt` expectation depend on it - so it is worth
one sentence in the `toTrinoException` javadoc that the passthrough exists for
`TrinoException`s raised inside the guarded block, not only for ones handed up
from below.
Verified by: reading CatalogConnectorMetadata.java:243-276,
DropCatalogStoredProcedure.java:96-150 and GravitinoErrorCode.java:122-135 in
this checkout.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
