Skip to content

refactor(pd): make the RaftEngine constructor private - #3380

Open
byteayan wants to merge 1 commit into
apache:masterfrom
byteayan:claude/pd-raftengine-private-ctor
Open

byteayan wants to merge 1 commit into
apache:masterfrom
byteayan:claude/pd-raftengine-private-ctor

Conversation

@byteayan

Copy link
Copy Markdown
Contributor

Purpose of the PR

RaftEngine is a singleton reached through RaftEngine.getInstance(), but its constructor is public, so code can create a second engine that never joins the raft group.

Main Changes

  • RaftEngine() is now private.
  • RaftEngineReadinessTest and RaftKVStoreTest built private engines with new RaftEngine(); they now use the same reflective newEngine() helper PDNativeOwnerTest already uses. Their behaviour is unchanged.

Nothing else constructs RaftEngine: production code and other tests use getInstance() or mock(RaftEngine.class) (Mockito creates mocks without calling the constructor). Store only depends on RaftReflectionUtil from hg-pd-core.

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as RaftEngineReadinessTest, RaftKVStoreTest, PDNativeOwnerTest.
  • Need tests and can be verified as follows:

Ran locally: mvn test -pl hugegraph-pd/hg-pd-test -am -P pd-core-test (190 tests, 0 failures, 2 skipped) and mvn editorconfig:check on hg-pd-core and hg-pd-test.

Does this PR potentially affect the following parts?

  • Dependencies
  • Modify configurations
  • The public API
  • Other affects (typed here)
  • Nope

Documentation Status

  • Doc - TODO: required documentation is pending; complete it before merging.
  • Doc - Done: documentation is included here or linked below.
  • Doc - No Need: no user-visible documentation is affected.

Documentation files in this PR or paired hugegraph-doc PR:

🤖 Generated with Claude Code

RaftEngine is a singleton reached through getInstance(). Tests that need
a private engine build it reflectively, as PDNativeOwnerTest already does.

Fixes apache#2655
@codecov

codecov Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.93%. Comparing base (c2ec570) to head (4e39665).
⚠️ Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##             master    #3380    +/-   ##
==========================================
  Coverage     55.93%   55.93%            
- Complexity     2277     2313    +36     
==========================================
  Files           774      776     +2     
  Lines         68534    68865   +331     
  Branches       9333     9414    +81     
==========================================
+ Hits          38334    38523   +189     
- Misses        26634    26760   +126     
- Partials       3566     3582    +16     
Flag Coverage Δ *Carryforward flag
commons 96.37% <ø> (ø) Carriedforward from c2ec570
hstore 35.97% <ø> (+0.16%) ⬆️
pd 49.41% <ø> (+0.23%) ⬆️
server_hbase 41.84% <ø> (-0.06%) ⬇️ Carriedforward from c2ec570
server_memory 54.29% <ø> (-0.04%) ⬇️ Carriedforward from c2ec570
server_rocksdb 42.29% <ø> (-0.06%) ⬇️ Carriedforward from c2ec570
store 31.97% <ø> (+0.04%) ⬆️

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 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.

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Blocking: no. Summary: Making the RaftEngine constructor private is correct and complete at this head; the only callers of new RaftEngine() outside the class were RaftKVStoreTest and RaftEngineReadinessTest, and both now build a private engine through the same reflective helper PDNativeOwnerTest already uses, so their behaviour is unchanged. Evidence: read the full diff at 4e39665; searched the whole repository at this head for new RaftEngine, subclasses and spy(RaftEngine...) (only the singleton field initializer remains; other tests use mock(RaftEngine.class), which does not call the constructor), found no RaftEngine references outside hugegraph-pd and no Spring or config-based instantiation; both changed tests are in PDCoreSuiteTest, which the pd_store / pd job runs with -P pd-core-test, and that job passed at this head along with every other non-skipped check.

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: no. Summary: The private-constructor change matches the singleton usage in this repository, and both isolated test constructions remain supported through reflection. Evidence: Exact-head diff and repository-wide constructor search; workflow run 39465275148 completed successfully with the PD job passing. No local tests were run. Score: 8.5/10

@byteayan
byteayan force-pushed the claude/pd-raftengine-private-ctor branch from 4e39665 to b57bc5c Compare October 21, 2026 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] pd make the RaftEngine constructor private

3 participants