nevzheng commented on code in PR #11959:
URL: https://github.com/apache/gravitino/pull/11959#discussion_r3562820490
##########
server/src/main/java/org/apache/gravitino/server/web/rest/ExceptionHandlers.java:
##########
@@ -1095,6 +1095,14 @@ public Response handle(OperationType op, String object,
String parent, Exception
String errorMsg =
getBaseErrorMsg(formattedObject, op.name(), formattedParent,
getErrorMsg(e));
+
+ // A backend a catalog federates to being unreachable is a
downstream-dependency failure, not
+ // an internal Gravitino error: surface it as 502 Bad Gateway so callers
can tell a dependency
+ // outage from a server bug.
+ if (e instanceof ConnectionFailedException) {
+ return Utils.connectionFailed(errorMsg, e);
+ }
Review Comment:
Good catch — fixed in c8d65acd4. Added `LOG.warn(errorMsg, e)` before the
502 return, mirroring the sibling `CatalogExceptionHandler`. WARN rather than
ERROR: a downstream dependency outage isn't a Gravitino bug, but it should
still leave a server-side trace.
##########
clients/client-java/src/main/java/org/apache/gravitino/client/ErrorHandlers.java:
##########
@@ -1374,6 +1374,9 @@ public ErrorResponse parseResponse(int code, String json,
ObjectMapper mapper) {
@Override
public void accept(ErrorResponse errorResponse) {
+ if (errorResponse.getCode() == ErrorConstants.CONNECTION_FAILED_CODE) {
+ throw new ConnectionFailedException("%s",
formatErrorMessage(errorResponse));
+ }
Review Comment:
Kept the `"%s"` here deliberately. The plain-message form is only free
inside handlers that carry a class-level
`@SuppressWarnings("FormatStringAnnotation")` — as `CatalogErrorHandler` does
(line 550), which is why its `new ConnectionFailedException(errorMessage)`
compiles. `RestErrorHandler` is the un-suppressed base, so passing a
non-constant string as the sole format argument to the `@FormatMethod`
constructor trips error-prone.
So the choice is: this literal `"%s"` (lint-clean, no suppression), or the
plain form plus a class-level suppression on the base handler that disables the
check for every throw in it. I'd rather keep the lint active than trade it for
cosmetics — and `"%s"` matches the `"Unable to process: %s"` line just below.
--
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]
