lasdf1234 commented on code in PR #12999: URL: https://github.com/apache/gravitino/pull/12999#discussion_r3965393127
########## 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: Thanks for the detailed repro — agreed this belonged in the PR. Fixed in 95c8c43bb: `usefulMessage` now prefers the shallowest non-blank (non-transparent) message and only appends a deeper reason when it is not already contained. Nested `wrap(...)` therefore keeps intermediate context such as the property name. Also added `testNestedWrapPreservesIntermediatePropertyContext`. ########## 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: Good catch. Fixed in 95c8c43bb by tracking visited throwables with an `IdentityHashMap`-backed set and stopping on re-entry. Added two-node and three-node cycle tests. ########## 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: Fixed in 95c8c43bb: candidate selection now uses `StringUtils.isNotBlank`, so whitespace-only deeper messages no longer overwrite useful outer reasons. Added blank-cause and blank-only-chain coverage. -- 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]
