Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
szetszwo merged PR #1341: URL: https://github.com/apache/ratis/pull/1341 -- 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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
taklwu commented on code in PR #1341:
URL: https://github.com/apache/ratis/pull/1341#discussion_r2921626120
##
ratis-common/src/main/java/org/apache/ratis/trace/TraceConfigKeys.java:
##
@@ -0,0 +1,45 @@
+/**
+ * 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 org.apache.ratis.conf.RaftProperties;
+
+import java.util.function.Consumer;
+
+import static org.apache.ratis.conf.ConfUtils.getBoolean;
+import static org.apache.ratis.conf.ConfUtils.setBoolean;
+
+public interface TraceConfigKeys {
Review Comment:
at the beginning it was part of the RaftServerConfigKeys in ratis-server,
but I was thinking to have this configuration on the client as well, such I
moved it to the ratis-common.
anyhow, as you said let's keep it temporarily.
--
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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
szetszwo commented on code in PR #1341:
URL: https://github.com/apache/ratis/pull/1341#discussion_r2921458677
##
ratis-common/src/main/java/org/apache/ratis/trace/RatisAttributes.java:
##
@@ -0,0 +1,36 @@
+/*
+ * 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 io.opentelemetry.api.common.AttributeKey;
+
+/**
+ * The constants in this class correspond with the guidance outlined by the
OpenTelemetry https://github.com/open-telemetry/semantic-conventions";>Semantic
+ * Conventions.
+ */
+public final class RatisAttributes {
+ public static final AttributeKey ATTR_CLIENT_INVOCATION_ID =
+ AttributeKey.stringKey("raft.client.invocation.id");
Review Comment:
It should be "raft.client.id".
##
ratis-common/src/main/java/org/apache/ratis/trace/RatisAttributes.java:
##
@@ -0,0 +1,36 @@
+/*
+ * 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 io.opentelemetry.api.common.AttributeKey;
+
+/**
+ * The constants in this class correspond with the guidance outlined by the
OpenTelemetry https://github.com/open-telemetry/semantic-conventions";>Semantic
+ * Conventions.
+ */
+public final class RatisAttributes {
+ public static final AttributeKey ATTR_CLIENT_INVOCATION_ID =
+ AttributeKey.stringKey("raft.client.invocation.id");
+ public static final AttributeKey ATTR_MEMBER_ID =
AttributeKey.stringKey("raft.member.id");
+ public static final AttributeKey ATTR_CALL_ID =
AttributeKey.stringKey("raft.call.id");
Review Comment:
Since the fields are already in RatisAttributes, let remove the `ATTR_`
prefix, i.e.
- RatisAttributes.MEMBER_ID
- RatisAttributes.CLIENT_ID
- RatisAttributes.CALL_ID
##
ratis-common/src/main/java/org/apache/ratis/trace/TraceConfigKeys.java:
##
@@ -0,0 +1,45 @@
+/**
+ * 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 org.apache.ratis.conf.RaftProperties;
+
+import java.util.function.Consumer;
+
+import static org.apache.ratis.conf.ConfUtils.getBoolean;
+import static org.apache.ratis.conf.ConfUtils.setBoolean;
+
+public interface TraceConfigKeys {
Review Comment:
Let's have this class temporarily. We should combine it with
RaftServerConfigKyes.
--
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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
taklwu commented on code in PR #1341: URL: https://github.com/apache/ratis/pull/1341#discussion_r2920867773 ## ratis-server/pom.xml: ## @@ -84,6 +84,18 @@ mockito-core test + + + io.opentelemetry + opentelemetry-sdk + test + + + io.opentelemetry + opentelemetry-sdk-testing + test Review Comment: ah, I missed that, I moved them all into `ratis-common` as transitive dependencies. -- 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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
szetszwo commented on code in PR #1341: URL: https://github.com/apache/ratis/pull/1341#discussion_r2920102860 ## ratis-server/pom.xml: ## @@ -84,6 +84,18 @@ mockito-core test + + + io.opentelemetry + opentelemetry-sdk + test + + + io.opentelemetry + opentelemetry-sdk-testing + test Review Comment: @taklwu , could you take a look https://issues.apache.org/jira/secure/attachment/13081062/1341_review.patch ? It does not add any dependencies to ratis-server. -- 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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
szetszwo commented on PR #1341: URL: https://github.com/apache/ratis/pull/1341#issuecomment-4035290891 @taklwu , thanks for the update! I will review it in details tomorrow. -- 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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
taklwu commented on code in PR #1341:
URL: https://github.com/apache/ratis/pull/1341#discussion_r2908032961
##
ratis-server/src/test/java/org/apache/ratis/server/impl/RaftServerImplTracingTests.java:
##
@@ -0,0 +1,141 @@
+/*
+ * 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.server.impl;
+
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.sdk.testing.junit5.OpenTelemetryExtension;
+import io.opentelemetry.sdk.trace.data.SpanData;
+import org.apache.ratis.conf.RaftProperties;
+import org.apache.ratis.protocol.ClientId;
+import org.apache.ratis.protocol.RaftClientRequest;
+import org.apache.ratis.protocol.RaftGroup;
+import org.apache.ratis.protocol.RaftGroupId;
+import org.apache.ratis.protocol.RaftPeerId;
+import org.apache.ratis.protocol.exceptions.ServerNotReadyException;
+import org.apache.ratis.server.storage.RaftStorage;
+import org.apache.ratis.statemachine.StateMachine;
+import org.apache.ratis.statemachine.impl.SimpleStateMachine4Testing;
+import org.apache.ratis.trace.TraceConfigKeys;
+import org.apache.ratis.trace.TraceUtils;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.mock;
+
+public class RaftServerImplTracingTests {
Review Comment:
I tried my best to avoid the otel dependency in ratis-server but I found
that needs as test dependency, can you let me know if this is fine ?
--
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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
taklwu commented on code in PR #1341:
URL: https://github.com/apache/ratis/pull/1341#discussion_r2908035478
##
ratis-server/pom.xml:
##
@@ -84,6 +84,18 @@
mockito-core
test
+
+
+ io.opentelemetry
+ opentelemetry-sdk
+ test
+
+
+ io.opentelemetry
+ opentelemetry-sdk-testing
+ test
Review Comment:
unless I test differently, otherwise, I see these lines are required...
##
ratis-server/src/test/java/org/apache/ratis/server/impl/RaftServerImplTracingTests.java:
##
@@ -0,0 +1,141 @@
+/*
+ * 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.server.impl;
+
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.sdk.testing.junit5.OpenTelemetryExtension;
+import io.opentelemetry.sdk.trace.data.SpanData;
+import org.apache.ratis.conf.RaftProperties;
+import org.apache.ratis.protocol.ClientId;
+import org.apache.ratis.protocol.RaftClientRequest;
+import org.apache.ratis.protocol.RaftGroup;
+import org.apache.ratis.protocol.RaftGroupId;
+import org.apache.ratis.protocol.RaftPeerId;
+import org.apache.ratis.protocol.exceptions.ServerNotReadyException;
+import org.apache.ratis.server.storage.RaftStorage;
+import org.apache.ratis.statemachine.StateMachine;
+import org.apache.ratis.statemachine.impl.SimpleStateMachine4Testing;
+import org.apache.ratis.trace.TraceConfigKeys;
+import org.apache.ratis.trace.TraceUtils;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.mock;
+
+public class RaftServerImplTracingTests {
Review Comment:
I tried my best to not include the otel dependency but I found that my test
still needs it as test dependency, can you let me know if this is fine ?
--
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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
taklwu commented on code in PR #1341:
URL: https://github.com/apache/ratis/pull/1341#discussion_r2898369270
##
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.
+ *
+ * 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 void addListener(CompletableFuture future,
+ BiConsumer 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:
reusing existing method is good, sorry that I didn't know
`unwrapCompletionException` was there.
--
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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
taklwu commented on code in PR #1341:
URL: https://github.com/apache/ratis/pull/1341#discussion_r2898368013
##
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 {
Review Comment:
I will give a try, it may take a bit.
but the idea was from the hbase implementation that few span builder was
created.
##
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 {
Review Comment:
I will give a try, it may take a bit.
but the idea was from the hbase implementation that few span builders was
created.
--
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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
szetszwo commented on code in PR #1341:
URL: https://github.com/apache/ratis/pull/1341#discussion_r2897405543
##
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 {
Review Comment:
https://github.com/user-attachments/assets/45b1e2b0-1a7c-4b0b-883b-7d3f02269432";
/>
We should use lambda and not to implement functional interface in general.
See also https://www.oracle.com/technical-resources/articles/java/lambda.html
--
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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
szetszwo commented on code in PR #1341: URL: https://github.com/apache/ratis/pull/1341#discussion_r2897468656 ## 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 (e.g. Ratis supports both Dropwizard Metrics v3 and v4 by making it pluggable). Another reason is that a dependency may have CVEs (e.g. the infamous [Log4Shell](https://en.wikipedia.org/wiki/Log4Shell) case). Applications may choose to disable it. BTW, we should add a conf to enable/disable tracing. -- 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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
szetszwo commented on code in PR #1341: URL: https://github.com/apache/ratis/pull/1341#discussion_r2897468656 ## 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 (e.g. Ratis supports both Dropwizard Metrics v3 and v4 by making it pluggable). Another reason is that a dependency may have CVE (e.g. the infamous [Log4Shell](https://en.wikipedia.org/wiki/Log4Shell) case). -- 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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
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.
+ *
+ * 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 void addListener(CompletableFuture future,
+ BiConsumer 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.apa
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
taklwu commented on PR #1341: URL: https://github.com/apache/ratis/pull/1341#issuecomment-4007673437 retest -- 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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
taklwu commented on PR #1341: URL: https://github.com/apache/ratis/pull/1341#issuecomment-4007261745 sorry that I rebased on top of master and messed up the review history, but the last [commit](https://github.com/apache/ratis/pull/1341/commits/d097a319ce791b797fe46140f3a295ded51487c2) is the only change for addressing the recent comments by @szetszwo . -- 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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
szetszwo commented on code in PR #1341:
URL: https://github.com/apache/ratis/pull/1341#discussion_r2885257518
##
ratis-common/src/main/java/org/apache/ratis/trace/RatisAttributes.java:
##
@@ -0,0 +1,34 @@
+/*
+ * 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 io.opentelemetry.api.common.AttributeKey;
+
+/**
+ * The constants in this class correspond with the guidance outlined by the
OpenTelemetry https://github.com/open-telemetry/semantic-conventions";>Semantic
+ * Conventions.
+ */
+public final class RatisAttributes {
+ public static final AttributeKey ATTR_MEMBER_ID =
AttributeKey.stringKey("raft.member.id");
+ public static final AttributeKey ATTR_CALLER_ID =
AttributeKey.stringKey("raft.caller.id");
Review Comment:
ClientInvocationId is a Client Id with either a call id or a stream id.
So, we may have
- "raft.client.id"
- "raft.call.id" (to be consistent, let's call it "call id")
- "raft.stream.id" (may be added later)
--
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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
taklwu commented on code in PR #1341:
URL: https://github.com/apache/ratis/pull/1341#discussion_r2885221790
##
ratis-common/src/main/java/org/apache/ratis/trace/RatisAttributes.java:
##
@@ -0,0 +1,34 @@
+/*
+ * 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 io.opentelemetry.api.common.AttributeKey;
+
+/**
+ * The constants in this class correspond with the guidance outlined by the
OpenTelemetry https://github.com/open-telemetry/semantic-conventions";>Semantic
+ * Conventions.
+ */
+public final class RatisAttributes {
+ public static final AttributeKey ATTR_MEMBER_ID =
AttributeKey.stringKey("raft.member.id");
+ public static final AttributeKey ATTR_CALLER_ID =
AttributeKey.stringKey("raft.caller.id");
Review Comment:
let me add `ClientInvocationId` as another attribute key
--
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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
taklwu commented on code in PR #1341:
URL: https://github.com/apache/ratis/pull/1341#discussion_r2880806037
##
ratis-common/src/main/java/org/apache/ratis/trace/TraceUtils.java:
##
@@ -0,0 +1,85 @@
+/*
+ * 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 io.opentelemetry.api.GlobalOpenTelemetry;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.api.trace.Tracer;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.context.propagation.TextMapPropagator;
+import io.opentelemetry.context.propagation.TextMapGetter;
+import org.apache.ratis.proto.RaftProtos;
+import org.apache.ratis.util.VersionInfo;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+
+public final class TraceUtils {
+
+ private TraceUtils() {
+ }
+
+ public static Tracer getGlobalTracer() {
+return GlobalOpenTelemetry.getTracer("org.apache.ratis",
VersionInfo.getSoftwareInfoVersion());
+ }
+
+ /**
+ * Create a span which parent is from remote, i.e, passed through rpc.
+ *
+ * We will set the kind of the returned span to {@link SpanKind#SERVER}, as
this should be the top
+ * most span at server side.
+ */
+ public static Span createRemoteSpan(String name, Context ctx) {
+return
getGlobalTracer().spanBuilder(name).setParent(ctx).setSpanKind(SpanKind.SERVER)
+.startSpan();
+ }
+
+ private static final TextMapPropagator PROPAGATOR =
+ GlobalOpenTelemetry.getPropagators().getTextMapPropagator();
+
+ public static RaftProtos.SpanContextProto injectContextToProto(Context
context) {
+Map carrier = new HashMap<>();
+PROPAGATOR.inject(context, carrier, (map, key, value) -> map.put(key,
value));
Review Comment:
any associated key value from the client span will be injected, in the next
PR , we will have `OPERATION_NAME`, `OPERATION_TYPE`, `PEER_ID`, `RPC_METHOD`
and etc.
--
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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
szetszwo commented on code in PR #1341:
URL: https://github.com/apache/ratis/pull/1341#discussion_r2874076760
##
ratis-common/src/main/java/org/apache/ratis/trace/RatisAttributes.java:
##
@@ -0,0 +1,34 @@
+/*
+ * 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 io.opentelemetry.api.common.AttributeKey;
+
+/**
+ * The constants in this class correspond with the guidance outlined by the
OpenTelemetry https://github.com/open-telemetry/semantic-conventions";>Semantic
+ * Conventions.
+ */
+public final class RatisAttributes {
+ public static final AttributeKey ATTR_MEMBER_ID =
AttributeKey.stringKey("raft.member.id");
+ public static final AttributeKey ATTR_CALLER_ID =
AttributeKey.stringKey("raft.caller.id");
Review Comment:
The callId is unique only within a single client. We should use
`ClientInvocationId`.
--
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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
szetszwo commented on code in PR #1341:
URL: https://github.com/apache/ratis/pull/1341#discussion_r2874084628
##
ratis-common/src/main/java/org/apache/ratis/trace/TraceUtils.java:
##
@@ -0,0 +1,85 @@
+/*
+ * 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 io.opentelemetry.api.GlobalOpenTelemetry;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.api.trace.Tracer;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.context.propagation.TextMapPropagator;
+import io.opentelemetry.context.propagation.TextMapGetter;
+import org.apache.ratis.proto.RaftProtos;
+import org.apache.ratis.util.VersionInfo;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+
+public final class TraceUtils {
+
+ private TraceUtils() {
+ }
+
+ public static Tracer getGlobalTracer() {
+return GlobalOpenTelemetry.getTracer("org.apache.ratis",
VersionInfo.getSoftwareInfoVersion());
Review Comment:
Add a static field, so we don't have to create the same Tracer multiple
times.
```java
private static final Tracer TRACER =
GlobalOpenTelemetry.getTracer("org.apache.ratis",
VersionInfo.getSoftwareInfoVersion());
```
##
pom.xml:
##
@@ -189,6 +189,9 @@
3.25.8
1.75.0
+
+1.57.0
+1.37.0
Review Comment:
How about using the latest 1.59.0 and 1.40.0?
##
ratis-common/src/main/java/org/apache/ratis/trace/TraceUtils.java:
##
@@ -0,0 +1,85 @@
+/*
+ * 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 io.opentelemetry.api.GlobalOpenTelemetry;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.api.trace.Tracer;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.context.propagation.TextMapPropagator;
+import io.opentelemetry.context.propagation.TextMapGetter;
+import org.apache.ratis.proto.RaftProtos;
+import org.apache.ratis.util.VersionInfo;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+
+public final class TraceUtils {
+
+ private TraceUtils() {
+ }
+
+ public static Tracer getGlobalTracer() {
+return GlobalOpenTelemetry.getTracer("org.apache.ratis",
VersionInfo.getSoftwareInfoVersion());
+ }
+
+ /**
+ * Create a span which parent is from remote, i.e, passed through rpc.
+ *
+ * We will set the kind of the returned span to {@link SpanKind#SERVER}, as
this should be the top
+ * most span at server side.
+ */
+ public static Span createRemoteSpan(String name, Context ctx) {
+return
getGlobalTracer().spanBuilder(name).setParent(ctx).setSpanKind(SpanKind.SERVER)
+.startSpan();
+ }
+
+ private static final TextMapPropagator PROPAGATOR =
+ GlobalOpenTelemetry.getPropagators().getTextMapPropagator();
+
+ public static RaftProtos.SpanContextProto injectContextToProto(Context
context) {
+Map carrier = new HashMap<>();
+PROPAGATOR.inject(context, carrier, (map, key, value) -> map.put(key,
value));
Review Comment:
Question: What exactly will be injected, ATTR_MEMBER_ID and ATTR_CALLER_ID?
Anything else?
##
ratis-common/src/main/java/org/apache/ratis/trace/TraceUtils.java:
##
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
taklwu commented on PR #1341: URL: https://github.com/apache/ratis/pull/1341#issuecomment-3992142359 @OneSizeFitsQuorum I have updated the license and good catch. -- 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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
OneSizeFitsQuorum commented on PR #1341: URL: https://github.com/apache/ratis/pull/1341#issuecomment-3907672360 @taklwu Please add license for the failed ci! -- 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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
szetszwo commented on code in PR #1341:
URL: https://github.com/apache/ratis/pull/1341#discussion_r2783947853
##
ratis-proto/src/main/proto/Raft.proto:
##
@@ -121,6 +121,7 @@ message RaftRpcRequestProto {
uint64 timeoutMs = 13;
RoutingTableProto routingTable = 14;
SlidingWindowEntry slidingWindowEntry = 15;
+ SpanContextProto spanContext = 16;
Review Comment:
Sorry for the confusion. We use the lower numbers for the core fields (such
as the fields required by Raft). The higher numbers are for miscellaneous
things.
--
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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
taklwu commented on code in PR #1341:
URL: https://github.com/apache/ratis/pull/1341#discussion_r2783848201
##
ratis-proto/src/main/proto/Raft.proto:
##
@@ -121,6 +121,7 @@ message RaftRpcRequestProto {
uint64 timeoutMs = 13;
RoutingTableProto routingTable = 14;
SlidingWindowEntry slidingWindowEntry = 15;
+ SpanContextProto spanContext = 16;
Review Comment:
ack , originally I thought the number was skip from 5 to 12 for some reason,
then I was wondered we should save the gap between 5 and 12 for other important
fields (or for backward compatibility)
and let me add more lines into this commit today
--
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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
taklwu commented on code in PR #1341:
URL: https://github.com/apache/ratis/pull/1341#discussion_r2783848201
##
ratis-proto/src/main/proto/Raft.proto:
##
@@ -121,6 +121,7 @@ message RaftRpcRequestProto {
uint64 timeoutMs = 13;
RoutingTableProto routingTable = 14;
SlidingWindowEntry slidingWindowEntry = 15;
+ SpanContextProto spanContext = 16;
Review Comment:
ack , and let me add more lines into this commit today
--
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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
szetszwo commented on code in PR #1341:
URL: https://github.com/apache/ratis/pull/1341#discussion_r2778212167
##
ratis-proto/src/main/proto/Raft.proto:
##
@@ -121,6 +121,7 @@ message RaftRpcRequestProto {
uint64 timeoutMs = 13;
RoutingTableProto routingTable = 14;
SlidingWindowEntry slidingWindowEntry = 15;
+ SpanContextProto spanContext = 16;
Review Comment:
We should not use 16; see
https://protobuf.dev/programming-guides/proto3/#assigning :
> You should use the field numbers 1 through 15 for the most-frequently-set
fields. Lower field number values take less space in the wire format. ...
Let's use 11?
--
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]
Re: [PR] RATIS-2393 Add Span Context to RaftRpcRequestProto [ratis]
taklwu commented on PR #1341: URL: https://github.com/apache/ratis/pull/1341#issuecomment-3850269799 @szetszwo Hi Nicholas, should I directly open PRs to master branch ? in addition , the test failure doesn't seem to be related to my changes ``` [INFO] Running org.apache.ratis.netty.TestLeaderElectionWithNetty OpenJDK 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended Error: Tests run: 25, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 74.14 s <<< FAILURE! -- in org.apache.ratis.netty.TestLeaderElectionWithNetty Error: org.apache.ratis.netty.TestLeaderElectionWithNetty.testChangeListenerToFollower -- Time elapsed: 1.509 s <<< FAILURE! org.opentest4j.AssertionFailedError: expected: <0> but was: <1> at org.apache.ratis.server.impl.LeaderElectionTests.testChangeListenerToFollower(LeaderElectionTests.java:562) at java.base/java.lang.reflect.Method.invoke(Method.java:569) at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) ``` -- 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]
