yuqi1129 commented on code in PR #12999: URL: https://github.com/apache/gravitino/pull/12999#discussion_r3965299023
########## common/src/main/java/org/apache/gravitino/utils/ExceptionMessages.java: ########## @@ -0,0 +1,108 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.utils; + +import javax.annotation.Nullable; + +/** + * Helpers for preserving underlying system error messages when wrapping exceptions. + * + * <p>Catalog and server code often adds operation context when rethrowing. Context is useful, but + * it must not replace the upstream message that operators need to act on. + */ +public final class ExceptionMessages { + + private ExceptionMessages() {} + + /** + * Returns the most specific non-blank message from {@code throwable} or its cause chain. + * + * @param throwable the throwable to inspect, may be null + * @return the deepest useful message, or null if none is available + */ + @Nullable + public static String usefulMessage(@Nullable Throwable throwable) { + if (throwable == null) { + return null; + } + + String lastUseful = null; + Throwable current = throwable; + while (current != null) { + String message = current.getMessage(); + if (message != null && !message.isEmpty()) { + lastUseful = message; Review Comment: [P2] Preserve diagnostic context instead of always selecting the deepest message Every non-empty descendant message replaces the previous one, even when the intermediate exception contains the configuration key or operation needed to diagnose the failure. This also discards context added by an earlier call to this helper. The PR introduces exactly this nested wrapping structure in `FileSystemUtils.writeProperty()` and `createConfiguration()`. Reproducing that structure with the actual helper: ```java Throwable root = new IOException("write failed"); Throwable inner = ExceptionMessages.wrap( "Failed to write property: fs.s3a.endpoint", root); ExceptionMessages.wrap("Failed to create configuration", inner).getMessage(); ``` The result is `Failed to create configuration: write failed`; the property name added by the first wrap is lost. The full cause remains available in the stack trace, but preserving actionable information in the client-visible message is the purpose of this change. Please retain meaningful immediate-cause context and append additional lower-level reasons only when needed, rather than reducing the entire chain to its deepest message. Transparent wrappers such as `ExecutionException` can be unwrapped explicitly. Add a nested-wrap test that asserts the property name survives. This should be fixed in this PR. ########## common/src/main/java/org/apache/gravitino/utils/ExceptionMessages.java: ########## @@ -0,0 +1,108 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.utils; + +import javax.annotation.Nullable; + +/** + * Helpers for preserving underlying system error messages when wrapping exceptions. + * + * <p>Catalog and server code often adds operation context when rethrowing. Context is useful, but + * it must not replace the upstream message that operators need to act on. + */ +public final class ExceptionMessages { + + private ExceptionMessages() {} + + /** + * Returns the most specific non-blank message from {@code throwable} or its cause chain. + * + * @param throwable the throwable to inspect, may be null + * @return the deepest useful message, or null if none is available + */ + @Nullable + public static String usefulMessage(@Nullable Throwable throwable) { + if (throwable == null) { + return null; + } + + String lastUseful = null; + Throwable current = throwable; + while (current != null) { + String message = current.getMessage(); + if (message != null && !message.isEmpty()) { Review Comment: [P2] Do not let a blank message overwrite a useful one The method promises a non-blank message, but `!message.isEmpty()` accepts whitespace-only strings. A deeper blank message therefore replaces an actionable outer reason. With the actual helper: ```java Throwable cause = new IOException( "HMS connection refused", new IOException(" ")); ExceptionMessages.withCause("Failed to load table", cause); ``` The result is `Failed to load table: `, losing `HMS connection refused` entirely. Please use a non-blank check when selecting candidate messages, while continuing to traverse causes and preserving the original text of valid messages. Add cases for spaces/newlines, an entirely blank chain, and a valid outer message followed by blank causes. This should be fixed in this PR. ########## common/src/main/java/org/apache/gravitino/utils/ExceptionMessages.java: ########## @@ -0,0 +1,108 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.utils; + +import javax.annotation.Nullable; + +/** + * Helpers for preserving underlying system error messages when wrapping exceptions. + * + * <p>Catalog and server code often adds operation context when rethrowing. Context is useful, but + * it must not replace the upstream message that operators need to act on. + */ +public final class ExceptionMessages { + + private ExceptionMessages() {} + + /** + * Returns the most specific non-blank message from {@code throwable} or its cause chain. + * + * @param throwable the throwable to inspect, may be null + * @return the deepest useful message, or null if none is available + */ + @Nullable + public static String usefulMessage(@Nullable Throwable throwable) { + if (throwable == null) { + return null; + } + + String lastUseful = null; + Throwable current = throwable; + while (current != null) { + String message = current.getMessage(); + if (message != null && !message.isEmpty()) { + lastUseful = message; + } + Throwable cause = current.getCause(); + if (cause == null || cause == current) { Review Comment: [P2] Detect multi-node cycles in the cause chain `cause == current` only detects a direct self-reference. Java permits a longer cycle without a custom Throwable implementation: ```java Throwable a = new RuntimeException("a"); Throwable b = new RuntimeException("b"); a.initCause(b); b.initCause(a); ExceptionMessages.usefulMessage(a); ``` This call never terminates. In the probe, the subprocess was still running when killed after a two-second timeout. If a connector supplies such a chain, wrapping the original failure will instead keep the request thread busy indefinitely. I have not confirmed an existing connector produces this shape, but a shared exception utility should terminate for it. Please track visited throwables by identity, for example with `Collections.newSetFromMap(new IdentityHashMap<>())`, and stop at a previously visited instance. Add two-node and three-node cycle coverage. This is a small fix worth including here. -- 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]
