imbajin commented on code in PR #2906: URL: https://github.com/apache/incubator-hugegraph/pull/2906#discussion_r2544824327
########## hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftReflectionUtil.java: ########## @@ -0,0 +1,93 @@ +/* + * 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.hugegraph.pd.raft; + +import com.alipay.sofa.jraft.Node; +import com.alipay.sofa.jraft.ReplicatorGroup; +import com.alipay.sofa.jraft.core.Replicator; +import com.alipay.sofa.jraft.entity.PeerId; +import com.alipay.sofa.jraft.util.ThreadId; + +import lombok.extern.slf4j.Slf4j; + +@Slf4j +public class RaftReflectionUtil { + + public static Replicator.State getReplicatorState(Node node, PeerId peerId) { + if (node == null || peerId == null) { + return null; + } + + // Get ReplicatorGroup from Node + var clz = node.getClass(); + ReplicatorGroup replicateGroup = null; + try { + var f = clz.getDeclaredField("replicatorGroup"); + f.setAccessible(true); + try { + replicateGroup = (ReplicatorGroup)f.get(node); + } + finally { + f.setAccessible(false); + } + } + catch (NoSuchFieldException | IllegalAccessException e) { + log.info("getReplicatorGroup: error {}", e.getMessage()); + return null; + } + + if (replicateGroup == null) { + return null; + } + + ThreadId threadId = replicateGroup.getReplicator(peerId); + if (threadId == null) { + return null; + } + else { + Replicator r = (Replicator)threadId.lock(); + try { + if (r == null) { + return Replicator.State.Probe; + } + Replicator.State result = null; + + // Get state from Replicator + + var replicatorClz = r.getClass(); + try { + var f = replicatorClz.getDeclaredField("state"); + f.setAccessible(true); + try { + result = (Replicator.State)f.get(r); + } + finally { Review Comment: ⚠️ **资源管理改进 - 使用 try-finally 确保清理** 新代码正确地使用了 try-finally 块来确保： 1. `f.setAccessible(false)` 总是被调用，即使在发生异常时 2. `threadId.unlock()` 总是被调用，避免死锁 这是一个很好的改进，提高了代码的健壮性和安全性。 建议：考虑在异常处理时记录更详细的上下文信息（如 peerId），便于问题排查： ```suggestion log.error("Failed to get replicator state for peerId: {}, error: {}", peerId, e.getMessage()); ``` ########## hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftReflectionUtil.java: ########## @@ -0,0 +1,93 @@ +/* + * 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.hugegraph.pd.raft; + +import com.alipay.sofa.jraft.Node; +import com.alipay.sofa.jraft.ReplicatorGroup; +import com.alipay.sofa.jraft.core.Replicator; +import com.alipay.sofa.jraft.entity.PeerId; +import com.alipay.sofa.jraft.util.ThreadId; + +import lombok.extern.slf4j.Slf4j; + +@Slf4j +public class RaftReflectionUtil { + + public static Replicator.State getReplicatorState(Node node, PeerId peerId) { + if (node == null || peerId == null) { Review Comment: ⚠️ **增加了参数校验** 新方法在开始时添加了 null 检查： ```java if (node == null || peerId == null) { return null; } ``` 这是一个好的防御性编程实践，避免了潜在的 NullPointerException。 不过，原来的实现在某些路径上没有这个检查，这可能改变了方法的行为。建议确认调用方是否依赖原有的行为（例如，是否有地方期望 NPE 被抛出）。 -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
