Repository navigation
Conversation
- reject legacy prefixes and invalid mappings before Store registration - validate incoming snapshots before replacing databases or advancing the index - ship a read-only checker using the existing Store runtime - document and test logical recovery through existing HugeGraph Tools
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3399 +/- ##
============================================
+ Coverage 55.93% 56.96% +1.03%
- Complexity 2277 2800 +523
============================================
Files 774 777 +3
Lines 68534 71785 +3251
Branches 9333 10900 +1567
============================================
+ Hits 38334 40894 +2560
- Misses 26634 27189 +555
- Partials 3566 3702 +136
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The offline checker cannot inspect a supported Topling deployment with its effective configuration because its launcher omits the selected runtime. Evidence: Compared hugegraph-store-check.sh with start-hugegraph-store.sh and traced GraphIdCheckCommand.run into RocksDBRuntime.verify; validation was static, with bash -n passing for the new launcher.
| cd -- "$TOP" | ||
| exec "$JAVA" "${MODULE_OPTIONS[@]}" \ | ||
| -Dloader.main=org.apache.hugegraph.store.node.GraphIdCheckCommand \ | ||
| -cp "${JARS[0]}" org.springframework.boot.loader.PropertiesLauncher "$@" |
There was a problem hiding this comment.
rocksdb.provider=topling configuration and a prepared Topling runtime, this command still loads only the bundled standard RocksDB classes: unlike start-hugegraph-store.sh, it neither sources preload-topling.sh nor prepends TOPLING_RUNTIME_CLASSPATH. GraphIdCheckCommand.run() then calls RocksDBRuntime.verify(), which rejects that provider/runtime mismatch before inspecting any database, so the documented offline pre-upgrade check cannot run for these deployments. Please reuse the existing verified runtime selection and prepend its selected JAR to the PropertiesLauncher classpath, with launcher coverage for both providers.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The GraphID guard itself reads correctly and its 25 new tests pass when run directly, but this head breaks two CI jobs on its own: the Store Docker images no longer build because the packaged guide is excluded from the Docker context, and the new exit-on-startup-failure path fails the Store launcher test, which also stops CI from ever running the new test classes. The earlier Topling comment on hugegraph-store-check.sh line 41 still applies at f1a5030; the launcher is unchanged. Evidence: read the full diff plus RocksDBSession, GraphIdManager, InnerKeyCreator, SnapshotHandler, PartitionManager path layout and the dist scripts; compared the failing docker / docker-build and pd_store / store logs with passing runs of the same jobs on other PRs at this base; built the Store dist from this head and started it with no PD (exit 1 after PD unreachable); ran GraphIdSnapshotGuardTest, GraphIdSnapshotLevelsTest, LegacyGraphIdCheckerTest and GraphIdCheckCommandTest with -P store-core-test (25 run, 0 failures).
|
|
||
| <files> | ||
| <file> | ||
| <source>${project.basedir}/../../docs/graphid-upgrade.md</source> |
There was a problem hiding this comment.
Important: This entry breaks every Docker build that packages the Store. The root .dockerignore excludes **/*.md and docs/ (lines 60-61), so docs/graphid-upgrade.md is missing from the build context and hg-store-dist fails with Failed to create assembly: Error adding file to archive: /pkg/hugegraph-store/hg-store-dist/./../../docs/graphid-upgrade.md. That is the docker / docker-build failure on this head (target server-hstore). The same job passes on other PRs at this base, and hugegraph-store/Dockerfile uses the same COPY . . context, so the published Store image cannot be built either. Please re-include the file in .dockerignore after those two patterns (for example !docs/graphid-upgrade.md) or keep the guide out of the assembly, then confirm docker / docker-build passes.
| LoggerFactory.getLogger(StoreNodeApplication.class) | ||
| .error("HugeGraph Store startup failed; exiting with status 1", e); | ||
| // An incomplete Spring bootstrap can leave non-daemon native/Raft executors alive. | ||
| System.exit(1); |
There was a problem hiding this comment.
Important: This exit fires for every startup failure, not only the GraphID preflight. A Store started without a reachable PD used to leave the JVM running; now it exits with status 1, and test-start-hugegraph-store.sh still expects the JVM to stay up. That makes pd_store / store fail on this head: Test 2 reports script exited early — foreground mode is not blocking and no running JVM available to verify explicit -g g1, Test 3 reports Java process not found, and Tests 3 and 4 see exit 1 instead of 137 and 143. The same job passes on other PRs at this base. I built the dist from this head and started it with no PD: hgStoreNodeService init throws RuntimeException: PDException: PD unreachable, pd.peers=localhost:8686, Spring logs Application run failed, and this catch exits 1 within seconds. Because that launcher step fails, the later Run core test step (-P store-core-test, which holds the four new GraphID test classes) and the lifecycle tests never run in CI. If failing fast without PD is the intended behaviour, update Tests 2-4 to run against a reachable or fake PD so they still have a live JVM to check. Otherwise, limit the exit to the GraphID preflight failure. Either way, pd_store / store needs to pass.
…erver325-20261025
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Score: 9/10. Summary: The Store startup guard, incoming-snapshot guard, offline checker and its launcher read correctly, and the three earlier findings (Topling runtime in hugegraph-store-check.sh, .dockerignore for the packaged guide, exit only on the GraphID preflight) are fixed at this head. docker / docker-build, pd_store / store and the four new GraphID test classes pass on this head.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: Startup now locks every discovered DB version, and DatabaseLocks.add fails on a healthy DB that has CURRENT but no LOCK, which is the state loadSnapshot leaves until the partition reopens. The snapshot guard, offline checker, launcher and the earlier review fixes read correctly, and CI passes at this head.
| } | ||
| for (Path database : databases) { | ||
| requireCurrent(database); | ||
| locks.add(database); |
There was a problem hiding this comment.
Important: loadSnapshot moves a checkpoint (no LOCK file) into db/<partition>_<v1>, and only a later read-write open creates LOCK, so a Store stopped or crashed in between now fails startup here with a bare NoSuchFileException from toRealPath() on healthy data. In DatabaseLocks.add, skip locking when LOCK is absent, since no RocksDB writer can hold it, and add a test that starts against a checkpoint-made DB directory.
fix #3224
Before ➡️ After
Before: Legacy reserved GraphID rows can become invisible during upgrade allocation or Raft replay; replacing the JAR cannot undo persisted mappings.
After: Refuse affected storage before normal startup or snapshot activation, provide a read-only offline checker, and document verified logical recovery into an empty cluster.
Why
Preserve evidence and fail safely when row ownership cannot be proven instead of attempting automatic physical conversion.
Validation boundary
The author reports regression and isolated recovery checks, but original experiment logs/archives are unavailable. Recovery is limited to independently verified readable business graphs; ambiguous ownership, TTL/system state and rolling upgrades need separate validation. Production-scale and real-cluster legacy-snapshot rejection remain open. Paired website documentation is still required.
Source proposal: 🔗 hugegraph/hugegraph#325.