Skip to content

fix(pd): wait for a held metadata RocksDB lock at startup - #2

Draft
byteayan wants to merge 139 commits into
masterfrom
claude/intelligent-curie-k7rz0y
Draft

byteayan wants to merge 139 commits into
masterfrom
claude/intelligent-curie-k7rz0y

Conversation

@byteayan

Copy link
Copy Markdown
Owner

Purpose of the PR

A restarted PD can start before the previous PD JVM has released the metadata RocksDB LOCK (systemd restart, stop then start). Since apache#3368 a failed metadata open fails startup instead of leaving a half-started PD, but it fails immediately, so this restart race still needs an operator. apache#3368 left the retry as a TODO pointing at apache#3226.

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 fcc6448 and 0d0eb70.

Main Changes

  • HgKVStoreImpl.init() retries the open for up to 30 seconds (the time stop-hugegraph-pd.sh waits for the old JVM) while the failure is a lock conflict, backing off from 500 ms to 4 s because every RocksDB open attempt rotates the info LOG. After that, startup fails exactly as before (IllegalStateException → PDException → RocksDBException).
  • Lock conflicts are recognised by IOError plus RocksDB's lock text: While lock file (another process) or lock hold by current process (this JVM). Both strings are present in rocksdbjni 8.10.2. Other failures, including a LOCK file that cannot be created, are not retried.
  • Interruption restores the interrupt flag and fails startup with the lock error.
  • Snapshot reload keeps opening without a retry; it shares the same open path with a zero wait.
  • Docs: hugegraph-pd/docs/configuration.md (PD Core Settings) describes the wait and warns never to delete LOCK files while the old process may be alive; README.md links to it; api-reference.md no longer calls the retry pending. The two TODOs pointing at [Bug] PD reports "Started" and keeps serving after failing to open its RocksDB (lock held by the previous instance): every request fails, /v1/health stays 200, no retry, no exit apache/hugegraph#3226 are removed.

Known limitation: only the metadata store waits. PD opens it before the raft log store today (PDService.init), and the shutdown order closes Raft first, so the raft log LOCK is already free by then.

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • Need tests and can be verified as follows:
    • New HgKVStoreImplTest cases: a lock released after 1 s is waited for; a lock never released fails after the (shortened) timeout with the RocksDB cause; a non-lock failure is not retried.
    • mvn test -pl hugegraph-pd/hg-pd-test -am -P pd-core-test: 193 tests, 0 failures, 2 skipped. mvn editorconfig:check on hg-pd-core, hg-pd-test, hg-pd-service passes.
    • Manual check with the built distribution (bin/start-hugegraph-pd.sh -d false) while another process held pd_data/rocksdb/LOCK with fcntl:
      • held for 8 s: PD logged While lock file ..., opened the store once it was released, and /v1/health returned 200;
      • held for 60 s: PD gave up after 30 s, startup failed with Failed to open PD metadata, and the process exited with status 1.

Does this PR potentially affect the following parts?

  • Dependencies
  • Modify configurations
  • The public API
  • Other affects (typed here): PD startup can take up to 30 s longer when the metadata LOCK is held
  • 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: hugegraph-pd/docs/configuration.md, hugegraph-pd/docs/api-reference.md, hugegraph-pd/README.md. No new configuration key; the website PD pages still need to be checked for startup or LOCK guidance before merge.


Generated by Claude Code

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:59
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.
A restarted PD can start before the previous JVM has released the
metadata RocksDB LOCK, and then fails startup straight away. Retry the
open for up to 30 seconds while the failure is a lock conflict, then
fail startup as before. Other open failures are not retried.

Fixes apache#3226
Each RocksDB open attempt rotates the info LOG, so back off from 500 ms
to 4 s instead of retrying every 500 ms, and never sleep past the
deadline. Share the open-and-wrap path with snapshot reload, tighten the
tests, and correct the docs about how a failed start exits.
@github-actions

Copy link
Copy Markdown

@codecov-ai-reviewer review

Copy link
Copy Markdown
Owner Author

All test jobs pass on this head (PD, Store, HStore, Server, cluster-test, docker-build, CodeQL). Two checks are not caused by this change:

  • dependency_license / dependency-review: Dependency review is not supported on this repository. The fork has the Dependency graph turned off (Settings → Code security). It passes once that setting is on; upstream already has it.
  • compatibility / build-server-riscv64: the first attempt failed while downloading maven-site-plugin dependencies from Maven Central (Premature end of Content-Length delimited message body), before any project code ran. It is on its one re-run now.

The upstream PR for this branch is apache#3379.


Generated by Claude Code

Copy link
Copy Markdown
Owner Author

Update on compatibility / build-server-riscv64: the re-run failed too, for a different reason than the first attempt. The build, the RocksDB JNI check and the first Server smoke start all passed. The second Server start (server-e2e-smoke-verify) did not finish in time under the emulated riscv64 CPU, still initializing the Gremlin script engines (Starting HugeGraphServer failed at 18:20:30). The script then stopped it, which logs Failed to close retained graphs before task DB shutdown from TaskManager.closeRetainedGraphs.

This is not this PR's: the job builds only the Server, RocksDB backend and PD client modules, not hg-pd-core where this change lives, and the same job passed on #1 and #3 with the same base. On the upstream PR apache#3379 this job is skipped by its path filter. No fix exists yet. The only re-run is spent, so I'm not re-running it again.


Generated by Claude Code

@byteayan
byteayan force-pushed the claude/intelligent-curie-k7rz0y branch from 0d0eb70 to 1c09528 Compare October 21, 2026 07:19

Copy link
Copy Markdown
Owner Author

CI note for b57bc5c: build-server / Server memory tests (Java 17) failed on ConsumersTest.testCleanStopEndsLazyProducer:362 expected:<2> but was:<1>.

This failure is not caused by this PR. The test is in hugegraph-server/hugegraph-core (Consumers). This PR only changes hugegraph-pd (RaftEngine constructor visibility and two PD tests). The assertion depends on timing: reads ends at 1 when provide(0) already sees the consumer's StopExecution before the loop gets to i == 1. Upstream has no fix for it yet. I'll re-run the failed job once after the rest of the run finishes.

dependency-review fails only because the Dependency graph is disabled on this fork.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

10 participants