szetszwo commented on code in PR #1341: URL: https://github.com/apache/ratis/pull/1341#discussion_r2893397811
########## ratis-common/src/main/java/org/apache/ratis/util/FutureUtils.java: ########## @@ -0,0 +1,79 @@ +/* + * 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.ratis.util; + +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; +import java.util.function.BiConsumer; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Helper class for processing futures. + */ +public final class FutureUtils { + + private static final Logger LOG = LoggerFactory.getLogger(FutureUtils.class); + + private FutureUtils() { + } + + /** + * This is method is used when you just want to add a listener to the given future. We will call + * {@link CompletableFuture#whenComplete(BiConsumer)} to register the {@code action} to the + * {@code future}. Ignoring the return value of a Future is considered as a bad practice as it may + * suppress exceptions thrown from the code that completes the future, and this method will catch + * all the exception thrown from the {@code action} to catch possible code bugs. + * <p/> + * And the error phone check will always report FutureReturnValueIgnored because every method in + * the {@link CompletableFuture} class will return a new {@link CompletableFuture}, so you always + * have one future that has not been checked. So we introduce this method and add a suppress + * warnings annotation here. + */ + @SuppressWarnings("FutureReturnValueIgnored") + public static <T> void addListener(CompletableFuture<T> future, + BiConsumer<? super T, ? super Throwable> action) { + future.whenComplete((resp, error) -> { + try { + // See this post on stack overflow(shorten since the url is too long), + // https://s.apache.org/completionexception + // For a chain of CompletableFuture, only the first child CompletableFuture can get the + // original exception, others will get a CompletionException, which wraps the original + // exception. So here we unwrap it before passing it to the callback action. + action.accept(resp, unwrapCompletionException(error)); + } catch (Throwable t) { + LOG.error("Unexpected error caught when processing CompletableFuture", t); + } + }); + } + + /** + * Get the cause of the {@link Throwable} if it is a {@link CompletionException}. + */ + public static Throwable unwrapCompletionException(Throwable error) { Review Comment: We already have a similar method - https://github.com/apache/ratis/blob/3d9f5af376409de7e635bb67c7dfbeadc882c413/ratis-common/src/main/java/org/apache/ratis/util/JavaUtils.java#L289 Let's reuse it and move the other methods to JavaUtils. ########## ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java: ########## @@ -17,6 +17,7 @@ */ package org.apache.ratis.server.impl; +import io.opentelemetry.api.trace.Span; Review Comment: Sorry that I may not be clear in my last comment -- we should import io.opentelemetry only in ratis-common. Then, it will be easier to change the tracing dependency to be pluggable in the future. We often see that a library may introduce incompatible changes (such as Dropwizard Metrics v3 vs v4). Since Ratis itself is also a library, we don't want to force our consumer applications to use a particular dependency (Ratis supports both Dropwizard Metrics v3 and v4). Another reason is that a dependency may have CVE (e.g. the infamous [Log4Shell](https://en.wikipedia.org/wiki/Log4Shell) case). ########## ratis-common/src/main/java/org/apache/ratis/trace/ClientRequestSpanBuilder.java: ########## @@ -0,0 +1,87 @@ +/* + * 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.ratis.trace; + +import static org.apache.ratis.trace.RatisAttributes.ATTR_CALL_ID; +import static org.apache.ratis.trace.RatisAttributes.ATTR_CLIENT_INVOCATION_ID; +import static org.apache.ratis.trace.RatisAttributes.ATTR_MEMBER_ID; + +import io.opentelemetry.api.common.AttributeKey; +import io.opentelemetry.api.trace.Span; +import io.opentelemetry.api.trace.SpanBuilder; +import io.opentelemetry.api.trace.SpanKind; +import io.opentelemetry.context.Context; +import org.apache.ratis.proto.RaftProtos.SpanContextProto; +import org.apache.ratis.protocol.RaftClientRequest; + +import java.util.HashMap; +import java.util.Map; +import java.util.function.Supplier; + +/** + * Construct {@link Span} instances originating from the client request. + */ +public class ClientRequestSpanBuilder implements Supplier<Span> { Review Comment: <img width="642" height="216" alt="Image" src="https://github.com/user-attachments/assets/45b1e2b0-1a7c-4b0b-883b-7d3f02269432"; /> We should use lambda and not to implement functional interface in general. ########## ratis-common/src/main/java/org/apache/ratis/util/FutureUtils.java: ########## @@ -0,0 +1,79 @@ +/* + * 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.ratis.util; + +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; +import java.util.function.BiConsumer; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Helper class for processing futures. + */ +public final class FutureUtils { + + private static final Logger LOG = LoggerFactory.getLogger(FutureUtils.class); + + private FutureUtils() { + } + + /** + * This is method is used when you just want to add a listener to the given future. We will call + * {@link CompletableFuture#whenComplete(BiConsumer)} to register the {@code action} to the + * {@code future}. Ignoring the return value of a Future is considered as a bad practice as it may + * suppress exceptions thrown from the code that completes the future, and this method will catch + * all the exception thrown from the {@code action} to catch possible code bugs. + * <p/> + * And the error phone check will always report FutureReturnValueIgnored because every method in + * the {@link CompletableFuture} class will return a new {@link CompletableFuture}, so you always + * have one future that has not been checked. So we introduce this method and add a suppress + * warnings annotation here. + */ + @SuppressWarnings("FutureReturnValueIgnored") + public static <T> void addListener(CompletableFuture<T> future, + BiConsumer<? super T, ? super Throwable> action) { + future.whenComplete((resp, error) -> { + try { + // See this post on stack overflow(shorten since the url is too long), + // https://s.apache.org/completionexception + // For a chain of CompletableFuture, only the first child CompletableFuture can get the + // original exception, others will get a CompletionException, which wraps the original + // exception. So here we unwrap it before passing it to the callback action. + action.accept(resp, unwrapCompletionException(error)); + } catch (Throwable t) { + LOG.error("Unexpected error caught when processing CompletableFuture", t); + } + }); + } + + /** + * Get the cause of the {@link Throwable} if it is a {@link CompletionException}. + */ + public static Throwable unwrapCompletionException(Throwable error) { Review Comment: What do you think about this comment? -- 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]
