Skip to content

Conversation

@contrueCT
Copy link
Contributor

@contrueCT contrueCT commented Nov 12, 2025

Purpose of the PR

Main Changes

  • Fixed incorrect reflection parameter in RaftEngine.getState and PartitionEngine.getState method: changed f.get(this.raftNode) to f.get(r).
  • Extracted duplicate methods (getReplicatorGroup, getState, getReplicatorState) from RaftEngine and PartitionEngine into a new utility class RaftReflectionUtil in hugegraph-common.
  • This eliminates code duplication and improves resource management with try-finally blocks for reflection access and lock unlocking.

Does this PR potentially affect the following parts?

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. bug Something isn't working raft store Store module labels Nov 12, 2025
@contrueCT contrueCT changed the title fix: incorrect reflection parameter in RaftEngine.getState method fix(store): fix incorrect reflection parameter in RaftEngine.getState method Nov 12, 2025
@imbajin imbajin requested a review from Copilot November 13, 2025 08:12
@codecov
Copy link

codecov bot commented Nov 13, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.25%. Comparing base (41d0dbc) to head (732df8c).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.raftNode to r in the getState method
  • 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.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Nov 14, 2025
@contrueCT contrueCT changed the title fix(store): fix incorrect reflection parameter in RaftEngine.getState method fix: fix incorrect reflection parameter in RaftEngine.getState method Nov 14, 2025
@contrueCT contrueCT changed the title fix: fix incorrect reflection parameter in RaftEngine.getState method refactor(store): fix reflection parameter error and extract duplicate methods to RaftReflectionUtil Nov 14, 2025
@contrueCT
Copy link
Contributor Author

contrueCT commented Nov 14, 2025

After analysis, I discovered that the three methods getReplicatorGroup(), getState(Replicator replicator), and getReplicatorState(PeerId peerId) in RaftEngine and PartitionEngine are completely identical.
Furthermore, the first two methods serve solely as private methods invoked by getReplicatorState(PeerId peerId).

Therefore, I merged them into a single getReplicatorState(Node node, PeerId peerId) method and placed it in the new utility class RaftReflectionUtil.java at hugegraph-commons/hugegraph-common/src/main/java/org/apache/hugegraph/util/RaftReflectionUtil.java.
This eliminates code duplication and improves resource management with try-finally blocks for reflection access and lock unlocking.

@contrueCT contrueCT requested a review from imbajin November 14, 2025 14:28
@contrueCT contrueCT force-pushed the fix_getState(Replicator-r) branch from 9b4701c to ee5f52d Compare November 17, 2025 11:31
@contrueCT contrueCT requested a review from imbajin November 19, 2025 14:02
try {
result = (Replicator.State)f.get(r);
}
finally {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 资源管理改进 - 使用 try-finally 确保清理

新代码正确地使用了 try-finally 块来确保:

  1. f.setAccessible(false) 总是被调用,即使在发生异常时
  2. threadId.unlock() 总是被调用,避免死锁

这是一个很好的改进,提高了代码的健壮性和安全性。

建议:考虑在异常处理时记录更详细的上下文信息(如 peerId),便于问题排查:

Suggested change
finally {
log.error("Failed to get replicator state for peerId: {}, error: {}", peerId, e.getMessage());

</dependency>
<dependency>
<groupId>org.apache.hugegraph</groupId>
<artifactId>hg-pd-core</artifactId>
Copy link
Member

@imbajin imbajin Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

‼️ 模块依赖问题 - 需要架构评审

在 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>

潜在问题:

  1. 循环依赖风险:PartitionEngine 现在依赖 hg-pd-core 中的 RaftReflectionUtil。需要检查 hg-pd-core 是否依赖 hg-store-core,避免循环依赖
  2. 模块职责不清:RaftReflectionUtil 放在 hg-pd-core 中,但被 store 模块使用。考虑是否应该放在一个更通用的 common 模块中?
  3. 版本硬编码:版本号 1.7.0 是硬编码的,应该使用 ${project.version} 或在父 pom 中统一管理 (默认使用 ${revision} )

建议:

  • 确认这个新的依赖关系符合项目的模块架构设计, 是否应该把它放到一个更合适, 已被依赖引用的模块里?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 目前hg-pd-core和hg-store-core之间没有循环依赖问题;
  2. 对于RaftReflectionUtil的模块问题,之前排除了hugegraph-common(因为这样做需要给hugegraph-common引入过多依赖),并且hg-store-core和hg-pd-core没有共同依赖的模块,因此选择直接让hg-pd-core依赖hg-pd-core,暂时没有想到更好的实现。
  3. 已修改为使用${revision}。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 目前hg-pd-core和hg-store-core之间没有循环依赖问题;
  2. 对于RaftReflectionUtil的模块问题,之前排除了hugegraph-common(因为这样做需要给hugegraph-common引入过多依赖),并且hg-store-core和hg-pd-core没有共同依赖的模块,因此选择直接让hg-pd-core依赖hg-pd-core,暂时没有想到更好的实现。
  3. 已修改为使用${revision}。

嗯嗯, 合理的, 另外 revision 应该是不需要声明的, 参考一下 server 的父子模块, 它默认会使用项目级别的版本号 (revision)

}
}
catch (NoSuchFieldException | IllegalAccessException e) {
log.info("getReplicatorState: error {}", e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 日志信息不一致

在获取 Replicator state 时的错误日志中,方法名仍然写的是 "getReplicatorGroup",但实际应该是 "getReplicatorState":

log.info("getReplicatorState: error {}", e.getMessage());

建议统一日志格式,并考虑使用 log.warnlog.error 而不是 log.info,因为这是一个异常情况:

Suggested change
log.info("getReplicatorState: error {}", e.getMessage());
log.warn("Failed to get replicator state via reflection: {}", e.getMessage(), e);

同时建议在第 50 行的日志也做类似修改。

@contrueCT contrueCT requested a review from imbajin November 21, 2025 16:40
@imbajin
Copy link
Member

imbajin commented Nov 23, 2025

image

Copy link
Member

@imbajin imbajin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution~

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 24, 2025
@imbajin imbajin requested review from Pengzna and VGalaxies November 24, 2025 09:04
@github-project-automation github-project-automation bot moved this from In progress to In review in HugeGraph PD-Store Tasks Nov 27, 2025
@VGalaxies VGalaxies merged commit 9a3daf8 into apache:master Nov 27, 2025
13 checks passed
@github-project-automation github-project-automation bot moved this from In review to Done in HugeGraph PD-Store Tasks Nov 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer raft size:L This PR changes 100-499 lines, ignoring generated files. store Store module

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[Bug] Incorrect reflection parameter in RaftEngine.getState method

3 participants