refactor(store): fix reflection parameter error and extract duplicate methods to RaftReflectionUtil#2906
Conversation
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java
Outdated
Show resolved
Hide resolved
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2906 +/- ##
=============================================
+ Coverage 30.97% 93.25% +62.28%
+ Complexity 264 65 -199
=============================================
Files 802 9 -793
Lines 67564 267 -67297
Branches 8776 22 -8754
=============================================
- Hits 20927 249 -20678
+ Misses 44308 8 -44300
+ Partials 2329 10 -2319 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in reflection-based field access within the getState method in two files. The method was incorrectly using this.raftNode as the reflection target instead of the Replicator r parameter that was passed to it.
- Corrected reflection parameter from
this.raftNodetorin thegetStatemethod - Applied the same fix consistently across both affected modules
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/PartitionEngine.java | Fixed incorrect reflection parameter in getState method to use the passed Replicator r parameter instead of this.raftNode |
| hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java | Fixed incorrect reflection parameter in getState method to use the passed Replicator r parameter instead of this.raftNode |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
After analysis, I discovered that the three methods Therefore, I merged them into a single |
...aph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/util/RaftReflectionUtil.java
Outdated
Show resolved
Hide resolved
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java
Show resolved
Hide resolved
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftEngine.java
Show resolved
Hide resolved
9b4701c to
ee5f52d
Compare
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftReflectionUtil.java
Show resolved
Hide resolved
| try { | ||
| result = (Replicator.State)f.get(r); | ||
| } | ||
| finally { |
There was a problem hiding this comment.
新代码正确地使用了 try-finally 块来确保:
f.setAccessible(false)总是被调用,即使在发生异常时threadId.unlock()总是被调用,避免死锁
这是一个很好的改进,提高了代码的健壮性和安全性。
建议:考虑在异常处理时记录更详细的上下文信息(如 peerId),便于问题排查:
| finally { | |
| 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
Show resolved
Hide resolved
hugegraph-pd/hg-pd-core/src/main/java/org/apache/hugegraph/pd/raft/RaftReflectionUtil.java
Show resolved
Hide resolved
| </dependency> | ||
| <dependency> | ||
| <groupId>org.apache.hugegraph</groupId> | ||
| <artifactId>hg-pd-core</artifactId> |
There was a problem hiding this comment.
在 hg-store-core 的 pom.xml 中添加了对 hg-pd-core 的编译时依赖:
<dependency>
<groupId>org.apache.hugegraph</groupId>
<artifactId>hg-pd-core</artifactId>
<version>1.7.0</version>
<scope>compile</scope>
</dependency>潜在问题:
- 循环依赖风险:PartitionEngine 现在依赖 hg-pd-core 中的 RaftReflectionUtil。需要检查 hg-pd-core 是否依赖 hg-store-core,避免循环依赖
- 模块职责不清:RaftReflectionUtil 放在 hg-pd-core 中,但被 store 模块使用。考虑是否应该放在一个更通用的 common 模块中?
- 版本硬编码:版本号 1.7.0 是硬编码的,应该使用 ${project.version} 或在父 pom 中统一管理 (默认使用
${revision})
建议:
- 确认这个新的依赖关系符合项目的模块架构设计, 是否应该把它放到一个更合适, 已被依赖引用的模块里?
There was a problem hiding this comment.
- 目前hg-pd-core和hg-store-core之间没有循环依赖问题;
- 对于RaftReflectionUtil的模块问题,之前排除了hugegraph-common(因为这样做需要给hugegraph-common引入过多依赖),并且hg-store-core和hg-pd-core没有共同依赖的模块,因此选择直接让hg-pd-core依赖hg-pd-core,暂时没有想到更好的实现。
- 已修改为使用${revision}。
There was a problem hiding this comment.
- 目前hg-pd-core和hg-store-core之间没有循环依赖问题;
- 对于RaftReflectionUtil的模块问题,之前排除了hugegraph-common(因为这样做需要给hugegraph-common引入过多依赖),并且hg-store-core和hg-pd-core没有共同依赖的模块,因此选择直接让hg-pd-core依赖hg-pd-core,暂时没有想到更好的实现。
- 已修改为使用${revision}。
嗯嗯, 合理的, 另外 revision 应该是不需要声明的, 参考一下 server 的父子模块, 它默认会使用项目级别的版本号 (revision)
| } | ||
| } | ||
| catch (NoSuchFieldException | IllegalAccessException e) { | ||
| log.info("getReplicatorState: error {}", e.getMessage()); |
There was a problem hiding this comment.
🧹 日志信息不一致
在获取 Replicator state 时的错误日志中,方法名仍然写的是 "getReplicatorGroup",但实际应该是 "getReplicatorState":
log.info("getReplicatorState: error {}", e.getMessage());建议统一日志格式,并考虑使用 log.warn 或 log.error 而不是 log.info,因为这是一个异常情况:
| log.info("getReplicatorState: error {}", e.getMessage()); | |
| log.warn("Failed to get replicator state via reflection: {}", e.getMessage(), e); |
同时建议在第 50 行的日志也做类似修改。
imbajin
left a comment
There was a problem hiding this comment.
Thanks for your contribution~

Purpose of the PR
Main Changes
Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No Need