Skip to content

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

Draft
byteayan wants to merge 138 commits into
masterfrom
claude/pd-raftengine-private-ctor
Draft

byteayan wants to merge 138 commits into
masterfrom
claude/pd-raftengine-private-ctor

Conversation

@byteayan

@byteayan byteayan commented Oct 20, 2026 •

Copy link
Copy Markdown
Owner

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.

Note: this branch is based on current apache/hugegraph master (86c05ef). This fork's master is behind upstream, so until it is synced this PR also lists upstream commits; the change itself is the single commit b57bc5c.

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:

byteayan and others added 30 commits October 1, 2026 02:15
RedirectFilter only forwarded requests when
GlobalMasterInfo.supportElection() was true, and the only code that set
it was the role election path removed in apache#3244, which nothing had
called since apache#3082. The filter was still registered on 21 endpoint
methods and returned early on every request.

Remove RedirectFilter, RedirectFilterDynamicFeature and the
@RedirectMasterRole annotations, the election and master URL state in
GlobalMasterInfo, and GraphTransaction.queryServerInfos(), which read
the ~server vertices HugeServerInfo used to write and had no callers.
Remove the struct TokenGenerator and the struct AUTH_TOKEN_SECRET option.
Also remove PD's TokenUtil, the only other code using this class name,
which has no callers. PDConfig's PUBLISHED_SECRET_KEY check is unchanged.
---------

Co-authored-by: imbajin <jin@apache.org>
Align the standard RocksDB and JRaft runtime. Older JRaft calls APIs removed by RocksDB 8.x

- share the standard RocksDB 8.10.2 version
- use compatible JRaft 1.3.14 for all modules
- update Store metrics and dependency metadata
- add old-data JNI upgrade coverage and guidance
After a Java upgrade, CI reports build-server (memory, 17) while branch protection still waits for build-server (memory, 11). This PR introduces the fixed required check Server memory tests, so changing the runtime JDK no longer requires renaming the protection context.
Operators currently assemble the distributed HugeGraph Kubernetes deployment
and startup configuration themselves. This PR provides a Helm installation path
for PD, Store and Server, with persistent storage, authentication defaults and
optional Hubble.

---------

Co-authored-by: imbajin <jin@apache.org>
Build and run Server, PD and Store on Java 17 while publishing Commons/RPC as Java 11 bytecode. Retain TinkerPop 3.5.1, use the Groovy 2.5.23 compatibility pin, and preserve valid old graph-ID reservations.

---------

Co-authored-by: imbajin <jin@apache.org>
Part 2 of upgrade series. Preserve transaction count and predicate semantics while retaining TinkerPop 3.5.1

Stabilize query behavior on TinkerPop 3.5.1: pending removals take precedence in transaction-visible counts, unsupported mixed filters remain in one local HasStep, and count barriers, input multiplicity and iterator cleanup are preserved.

---------

Co-authored-by: imbajin <jin@apache.org>
Select affected tests and run them before compatibility and security work, while sharing image preparation.
Separate affected test coverage from merge requirements so maintainers can cancel advisory CI without blocking Memory, license checks or CodeQL startup.
…pache#3260)

The chart carries guards, probe derivations and runbook steps that compensate for server-side behavior rather than Kubernetes concerns. Each of those workarounds should disappear when the server behavior is fixed, and the only way a later change at the server site learns that is a comment at that site. This PR leaves one TODO per site, stating what is wrong or missing, what the proper fix does, and which chart guard is removed afterwards, linking the tracking issue where one exists.
- Upgrade TinkerPop and Groovy with aligned runtime dependencies
- Configure the default and Raft Gremlin servers for the upgraded serializers
- Update version checks and retain Groovy script compatibility coverage

Co-authored-by: imbajin <jin@apache.org>
- Adapt graph steps and count strategies to the upgraded traversal APIs
- Align predicate handling across core and shared condition types
- Update feature declarations and related graph and condition tests
- Adapt Cypher client and processor integration to the upgraded Driver APIs
- Update GraphSON modules and type serializer registration
- Retain Cypher and JSON serialization compatibility checks
- Run feature scenarios with isolated graph provider context and lifecycle
- Validate compatibility reports and retain API metrics coverage
- Integrate suite selection into Server and Store CI and cancel superseded runs
- Keep lifecycle RocksDB data and WAL within the test temporary directory

Co-authored-by: imbajin <jin@apache.org>
- Keep unsupported type predicates in local traversal filters
- Preserve native and UUID ID comparisons during local evaluation
- Add core and optimizer coverage for ID and count predicate behavior

Co-authored-by: contrueCT <contrue_ct@outlook.com>
- Align PD and Store YAML dependencies with the secured runtime
- Adapt Kubernetes and Store configuration readers to safe YAML construction
- Cover resource quotas and PD listener configuration and register unit tests

Co-authored-by: contrueCT <contrue_ct@outlook.com>
- Align bundled Driver and remote serializers with the upgraded Gremlin runtime
- Preserve typed result objects in Driver configurations
- Cover serializer configuration and secured YAML loading

Co-authored-by: contrueCT <contrue_ct@outlook.com>
- Explain supported TinkerPop behavior and concrete upgrade checks
- Clarify list processing and typed GraphSON configuration guidance
- Align build instructions and project version references

Co-authored-by: contrueCT <contrue_ct@outlook.com>
- Update release LICENSE and NOTICE for the upgraded runtime
- Consolidate third-party license additions, replacements and removals
- Align the packaged dependency inventory with the resolved runtime

Co-authored-by: imbajin <jin@apache.org>
- Restore finite GraphBinary results and legacy HTTP Tree output
- Preserve named bindings and local ID/count filter semantics
- Normalize DATE values and align YAML/logging dependencies
- Cover real regressions and paired upgrade documentation
- Guard current-head retries and retain HStore failure evidence
- remove unsafe typed GraphSON V1 from shipped configs
- preserve mixed edge ID membership and count semantics
- encode public enums and SchemaManager with standard wire values
- share compatible Cypher factories across imports and replay
- align Jackson modules and release dependency notices
- encode Shard results as standard GraphBinary maps
- mirror OffsetDateTime conversion in the shared graph model
- regenerate the exact runtime dependency inventory
- retain TP report gates in the new reusable CI layout
- limit the exception to ASF PR 3263 and its source branch
- keep unit, core and API tests with explicit TP skip summaries
- mark the temporary branch patch with a removal TODO
- make struct the canonical ID, schema, query, element and codec owner
- move schema network lifecycle to Store and JWT/RPC contracts to common
- preserve historical formats and repair bounded HStore OLAP operations
- migrate callers and add compatibility fixtures and package boundary guards
- document Java migration and refresh dependency inventory and licensing
- retain the original ASF header and third-party source attribution
- run historical Store ingress tests within their node module owner
- keep the client decoder dependency limited to the test classpath
- require the relocated CI tests and aggregate their coverage session
- prevent Python caches from entering source formatting checks
- restore the canonical exception import after merging master
- compare string IDs consistently across lazy representations
- cover Unicode ordering contracts and document the correction
- consume class descriptors once across input refills
- cover frozen legacy and canonical streamed payloads
- retain Docker build output alongside image IDs
imbajin and others added 25 commits October 16, 2026 00:42
- serialize request sends and terminal observer calls
- suppress requests and repeated terminals after completion
- preserve final pages, original errors and channel retirement
- cover completion and cancellation during in-flight sends
HG-19 follow-up to apache#3241, within apache#3242. Keep the response envelope compatible with existing Java clients and classify actual Cypher translator failures correctly. Semantic parser errors previously fell through to execution or unsupported-feature errors, while backend unsupported predicates could be mistaken for translator limitations.

This PR retains Nguyen Quang Minh's structured-error work and authorship from apache#3241. Its response metadata lives in status.attributes.errors, preserving the existing top-level fields, status and message.

ExecutionError may also cover unrecognized translation failures and does not imply traversal execution started; original messages are preserved and classification does not cover every translator failure.
When concurrent Loaders finish independently, one can restore NONE while another still imports. Keep LOADING until the last owner releases it on the same live graph instance, and provide controlled recovery when an import loses its handle.
HStore Gremlin schema requests can return 403 when a V2 schema task needs a new worker thread. The existing async-task permission mapping covers V1 but omits V2.
helm test can pass every Server while one Pod retains an unready index state and indexed queries fail. The hook previously checked only graph binding and a simple scan. It now checks visible linked index state on every Pod, with optional explicit business-query assertions.
…3357)

PD keeps every store's state and every shard group's leader in its metadata, but its Prometheus endpoint only exports how many stores exist. While a rebuilt Store recovers, an operator has to read logs or poll /v1/stores and /v1/shardGroups to see whether it is back Up and whether leaders have moved back to it. Because PD serves these gauges, they stay readable while the recovering Store cannot serve its own endpoint.
…apache#3352)

The Server entrypoint maps a set of HG_SERVER_* variables onto the conf files, but nothing mapped the two keys that decide which address a Server registers in PD: server.urls_to_pd and server.deploy_in_k8s. GraphManager registers the urls_to_pd list only when deploy_in_k8s is true, and restserver.url otherwise. Without a mapping, the Helm chart rewrites rest-server.properties in a shell wrapper before it runs the entrypoint, and the TODO in docker-entrypoint.sh asked for these mappings.
Gremlin Server opens the graphs in conf/graphs itself, once, inside HugeGremlinServer.prepare(). TinkerPop's DefaultGraphManager logs and drops any graph whose open throws. That open runs before the REST GraphManager exists, so it also runs before the pd.stores_wait_timeout wait. When PD is briefly unreachable at that point, the Gremlin open fails. A few seconds later REST opens the same graph successfully, after its wait. GraphManager.loadGraph(String, String) then stored the graph without firing GRAPH_CREATE, which is the only way ContextGremlinServer binds a graph after startup. The Server answered REST and passed readiness, and every Gremlin call on that graph failed with Could not rebind [DEFAULT-hugegraph] until the process restarted.
- rename the chart artifact to hugegraph-helm
- preserve Kubernetes application names and selectors
- document versioned OCI pulls and offline mirroring
- verify package metadata and update chart label fixtures
Graphs added after startup could miss cache RPC registration. Cleanup failures could retain stale owners, and a same-name ADD could be discarded while an older owner was still healthy or closing. Graphs now become visible after RPC registration, unfinished cleanup remains retryable, and received ADDs reload current metadata after the old owner is released.
Verify the remaining service after the first unexport.
Wait for listener closure after removing the last service.
Reuse the existing fixture without production changes.
The website now uses --devel for Helm trials, while the repository still requires an explicit version and shows a placeholder development build. Align the repository guides with the merged website update.
- set the chart icon to the HugeGraph logo
- reuse the SVG served by the project website
- display project branding in chart catalogs
With Server authentication disabled, both Cypher GET and POST requests still returned 401 unless an Authorization header was supplied. This also prevented Hubble from executing Cypher in anonymous mode. Cypher now follows the Server authentication switch and connects to Gremlin without credentials when authentication is disabled.
…3367)

Grouping by a nullable boolean property produces rows whose key is
true, false or the translator null marker, and ordering such rows once
failed with a ClassCastException on TinkerPop 3.5.x because the marker
compares as a string. With the TinkerPop versions built against now the
same query orders all three keys with Cypher null semantics (null first
descending, last ascending). Add an api-test that runs the issue's
query shape through the Cypher endpoint in both directions.

Co-authored-by: Rabari Mihir <169349389+Mihir-Rabari@users.noreply.github.com>
Arthas could previously be activated through unprotected HTTP endpoints, with diagnostic ports bound to 0.0.0.0. Server now requires configured authentication and an authenticated administrator for PUT /arthas; Store's unauthenticated /v1/arthasstart returns HTTP 403.
- replace the challenged badge endpoint with Shields
- preserve the DeepWiki label and repository link
- verify the replacement serves a valid SVG
The schema sync design asks for PD's consistent read, or the read barrier it needs, before the notify and ACK protocol is built on it (section 5.1). PD has no such barrier on master: get and scanPrefix read the local RocksDB with only an isLeader() check, and id allocation reads its counter the same way. This PR adds the barrier to id allocation and fixes two PD raft bugs found while checking that path. It does not depend on the schema sync work and is useful on its own.

---------

Co-authored-by: imbajin <jin@apache.org>
…e#3370)

The Cypher endpoint rejects the standard date() constructor with
Unknown function 'date' because the cypher-for-gremlin translator ships a
fixed function list that contains no temporal functions. That list lives
inside the translator dependency, so there is no way to register new
functions from HugeGraph; instead I fold the temporal constructors on the
HugeGraph side. Right before translation, date(), datetime() and
localdatetime() calls with a foldable argument (no argument, a string
literal, or a parameter bound to a string) are rewritten into a synthetic
bound parameter carrying a java.util.Date, the value HugeGraph already
uses for DATE properties.

---------

Signed-off-by: Rabari Mihir <169349389+Mihir-Rabari@users.noreply.github.com>
Co-authored-by: imbajin <jin@apache.org>
Root cause: RaftStateMachine.onLeaderStop() replaced a terminal error with follower state, allowing /v1/health to report success after an unrecoverable Raft failure. Queued leader/follower callbacks could also overwrite terminal state.

---------

Co-authored-by: imbajin <jin@apache.org>
clear() deletes the graph from the Store and from PD, but kept it in
infoInitializedGraph, and only open() registered a graph. When a running
graph is cleared and initialized again, its store sessions stay open, so
HstoreStore.open() returns early and init() created tables for a graph
PD no longer had.

Move the registration into registerGraph(), call it from open() and from
createTable(), which init() uses, and have clear() forget the
registration in a finally block.

Part of apache#3090
Global property queries can silently omit records from labels without compatible indexes. For type-compatible values, this change requires complete coverage for every query-visible candidate label and raises NoIndexException for partial coverage. Adding hasLabel(...) or compatible indexes on all candidates provides an explicit remedy.
RaftEngine is a singleton reached through getInstance(). Tests that need
a private engine build it reflectively, as PDNativeOwnerTest already does.

Fixes apache#2655
@github-actions

Copy link
Copy Markdown

@codecov-ai-reviewer review

Copy link
Copy Markdown
Owner Author

Two red checks here are not caused by this change:

  • compatibility / build-server-riscv64: Maven Central reset the connection while downloading org.rocksdb:rocksdbjni:8.10.2 (Could not transfer artifact ... Connection reset) during dependency resolution, before any test ran. I'll re-run the failed jobs once when the run finishes.
  • dependency_license / dependency-review: Dependency review is not supported on this repository. The fork has the Dependency graph turned off (Settings → Code security). No code fix applies; it passes once that setting is on, and upstream already has it.

Generated by Claude Code

Copy link
Copy Markdown
Owner Author

CodeQL reports 9 new alerts, but they come from this fork setup, not from this change. This fork's master is 137 commits behind apache/hugegraph, so the PR diff includes all of those upstream commits ("code changes were too large"), and CodeQL has never scanned the fork's master, so it has no baseline. This PR itself only makes RaftEngine() private and switches two tests to a reflective constructor helper. The check should compare correctly once the fork is synced, or on the upstream PR against apache/hugegraph master.


Generated by Claude Code

@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

10 participants