Replies: 2 comments 1 reply
Horizon / timelineI don't see this as a fire drill — but it's a compounding issue we should commit to resolving over roughly the next year, in three registers:
Why a real horizon and not "someday": I believe this will affect enterprise engagements. Large customers need proper error handling to deploy Gravitino into — and integrate it with — their existing systems; their automation and their on-call depend on it. On-call's whole job is to firefight, and you can't firefight when every failure is the same generic 500 with no signal of whose fault it is or whether to retry. Clean error semantics are what let their operators (and ours) act instead of guess. (Checking with Mark on how this has shown up in real engagements.) |
It depends on how we treat such data source issues. If we return 401/403, 1) It will mix with Gravitino's own authn/authz error, making it hard for the user to understand; 2) some systems will treat 401 as a specially case to do some additional process like relogin, reissue the tickets, which will not work for the data sources Gravitino connected. So it is debatable to change this semantics.
I think it makes sense. But, we don't support rate control / backpressure for now. We'd better adding rate control before we support these two error codes.
Again, itself makes sense. If we can differentiate these two errors, we can have two different codes.
This should be treated case by case. We'd better not add the retry codes for all the cases. We need to separate into case by case thing.
This is a good thing.
We can use OpenAPI to track the stability. But I cannot figure out a better solution. Maybe we can have more discussion on this. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
The problem
Gravitino is a gateway that federates external catalogs (Iceberg REST, Hive Metastore, JDBC, Kafka, filesets), but it handles failures with an overly-broad catch discipline. Across the catalog layer, the recurring shape is:
The server then renders that as HTTP 500 Internal Server Error. The consequence: three fundamentally different kinds of failure become indistinguishable to callers and to us in production:
Today all three look like the same generic 500. This is a recognized anti-pattern (CWE-396: Declaration of Catch for Generic Exception — broad catches "obscure the source of the exception and make debugging more difficult"), not just a style preference.
The data
throw new RuntimeException(...)in catalog code (catalogs/*/src/main, branchmain):~127 sites in catalog code (full
file:linelist attached). Not every site swallows a backend exception — some are genuine internal invariants — but the ones catchingException/IOException/SQLException/TExceptionaround a backend call are where dependency-vs-us and retriable-vs-terminal information is erased. Concretely today:RuntimeException.server/.../web/rest/ExceptionHandlers.java),ConnectionFailedException → 502is mapped by exactly one of 22 per-entity handlers (CatalogExceptionHandler); everywhere else it falls through to 500.UnauthorizedException(error code 1011) is emitted by no handler. There is noExceptionMapper<Throwable>safety net.Retry-Afterawareness;INTERNAL_ERRORbecomes a bareRuntimeException.Why this matters — and why it grows over time
Operationally / for deployments. We are a gateway; per RFC 9110 §15.6 an upstream outage is 502/503/504, not 500. Returning 500 for a dependency being down is a spec misuse that routes the wrong on-call to the wrong system. A 500 should mean "a Gravitino invariant broke" and should be rare — when it's the catch-all, real bugs hide inside a flood of dependency errors and every incident starts from zero.
For users / clients. Every backend we federate already ships a structural retriable signal — Kafka
RetriableException, JDBCSQLTransientException/SQLRecoverableException, IcebergServiceUnavailableException/503, HiveTTransportException— and we discard all of it at the adapter boundary. Clients can't safely auto-retry because they can't tell transient from permanent. (AIP-194: "retrying a request with an invalid argument will never succeed — only transient failures merit automatic retry.")For enterprise adoption and complex deployments (the commercial argument). Good error taxonomy is not cosmetic — it's what makes Gravitino operable inside someone else's production, which is exactly the bar for enterprise:
In one line: clean error semantics convert Gravitino from "a black box that returns 500" into "a well-behaved gateway component" — a procurement and integration checkbox, not a nice-to-have.
A possible direction (for discussion, not a decision)
The standard remedy is a small closed taxonomy where the category itself tells the caller who's at fault and whether to retry — the model used by gRPC status codes, Google AIP-193, and HTTP. Mapped to our gateway role, roughly four buckets + a
retriablebit:INVALID_ARGUMENT,NOT_FOUND,ALREADY_EXISTS,FAILED_PRECONDITIONUNAUTHENTICATED,PERMISSION_DENIEDRetry-After) / 504UNAVAILABLEINTERNAL)INTERNALGuiding principle: classify-then-preserve rather than swallow-then-whitelist — classify a failure once, at the adapter boundary where the native exception still carries meaning, and preserve that classification (incl. the
retriablebit) all the way to the HTTP response and the client.Two dimensions the bucket alone doesn't capture
Which bucket is only one axis. An error carries two more independent facets — both straight from the same standard playbook (Google's
google.rpc.Status/ErrorInfo, AbseilStatus):gravitino-corevsiceberg-restvshive-metastorevsjdbc:<vendor>. A machine-readable domain (AIP-193'sErrorInfo.domain+reason) lets callers and our own tooling attribute and route failures by blast radius instead of grepping message strings. For a gateway this is arguably the central concept — you cannot reason about "whose fault" without naming the domain. (Cf. "Ow! My Foot!" — "think in error domains and blast radius"; "error domains are everything.")Remediation plan (proposed, incremental)
Not a big-bang rewrite — each phase is independently shippable and ordered by value/cost. The goal is correct classification, not "zero
RuntimeExceptions": genuinely-internal failures (invariants, our ownEntityStore) should stay 500; we're separating those from dependency failures.ConnectionFailedException → 502,UnauthorizedException → 401,ForbiddenException → 403intoBaseExceptionHandlerso all 22 per-entity handlers inherit consistent mapping (today onlyCatalogExceptionHandlermaps 502).ExceptionMapper<Throwable>safety net so nothing escapes as an unparseable container 500.failure()(caller / dependency-unavailable / dependency-failure / internal),retriable(), anddomain.Retry-After), 504; wireUtils/ErrorConstants/ErrorResponse.ServiceFailure/ transport-IOException, and encode the traps (CommitStateUnknownException→ never retry;CommitFailedException→ retry whole commit).RetriableException), Fileset/HDFS, Paimon."Access denied", simple-name) to marker-based (SQLTransient*/ SQLState class /TTransportException).iceberg-rest,hive-metastore,jdbc:<vendor>,gravitino-core).Retry-After+ bounded backoff for idempotent operations; reverse-map the new codes in the base error handler.catch (Exception) → RuntimeExceptionin adapters.Open questions / other categories to work through
AuthorizationException, JDBC SQLState28, Iceberg 401/403) collapse to 500. How do we surface upstream authz failures vs Gravitino's own access control?Retry-After). Exists only for the lineage sink today — should the gateway propagate upstream throttling?CommitStateUnknownException→ never auto-retry (possible double-write);CommitFailedException→ retry the whole commit, not the request; KafkaUnknownTopicOrPartitionExceptionis marked retriable but is semantically 404; JDBC — classify on the marker hierarchy before SQLState (class08is ambiguous).ConnectionFailed/Unauthorized/ForbiddenintoBaseExceptionHandler+ add anExceptionMapper<Throwable>safety net) before the larger taxonomy + per-catalog converters + client retry?References
The anti-pattern: CWE-396 / CWE-397 (generic catch/throw) · CWE-209 (error-message info exposure) · Oracle Java Tutorials — Exceptions · Effective Java Item 77 · Nygard, Release It!
Error domains, public/internal split, and cross-language design: "Ow! My Foot!" — a survey of error handling across C, Go, Rust, and Abseil (nevzheng, 2026) — "think in error domains and blast radius"; "the silent swallow and the invisible rethrow"; "error domains are everything."
The taxonomy to adopt: gRPC status codes · AIP-193 Errors / AIP-194 Retry · RFC 9110 §15.6
Related: #11943, #11959 (the Iceberg REST
warehousefail-fast fix that surfaced this).Data appendix — full
file:linelist of the ~127 sites (click to expand)Generated from
grep -rn 'throw new RuntimeException' catalogs/*/src/main --include='*.java'on branch claude/dazzling-knuth-703829 @ 799c50c.Count = 127 across catalog modules. (Not every site swallows a backend exception — some are internal invariants; the ones that catch
Exception/IOException/SQLExceptionfrom a backend call are the ones that erase dependency/retriability semantics.)catalog-common (3)
catalog-fileset (19)
catalog-glue (5)
catalog-hive (24)
catalog-jdbc-common (1)
catalog-jdbc-doris (2)
catalog-jdbc-starrocks (1)
catalog-kafka (15)
catalog-lakehouse-generic (9)
catalog-lakehouse-hudi (4)
catalog-lakehouse-iceberg (5)
catalog-lakehouse-paimon (5)
catalog-model (14)
hadoop-common (5)
hive-metastore-common (15)
All reactions