feat(store): implement pluggable cloud storage for HStore#3081
feat(store): implement pluggable cloud storage for HStore#3081vaijosh wants to merge 10 commits into
Conversation
…age with S3 support out of the box
dosubot
Bot
added
size:XXL
apache#3081 -added multipart upload for the S3 storage provider
vaijosh
force-pushed
the
Hstore+CloudStorage
branch
from
6a3f496 to
8b6d597
Compare
apache#3081 -added multipart upload for the S3 storage provider
vaijosh
force-pushed
the
Hstore+CloudStorage
branch
from
8b6d597 to
4275426
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3081 +/- ##
============================================
- Coverage 36.80% 1.46% -35.34%
+ Complexity 338 21 -317
============================================
Files 805 788 -17
Lines 68587 66653 -1934
Branches 9029 8672 -357
============================================
- Hits 25241 975 -24266
- Misses 40653 65594 +24941
+ Partials 2693 84 -2609 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
apache#3081 - Improved test coverage.
VGalaxies
left a comment
VGalaxies
left a comment
There was a problem hiding this comment.
Review summary
- Blocking: yes
- Summary: The change cannot currently guarantee recoverable RocksDB state, may silently disable cloud storage, and contains S3 retry and deployment defects.
- Evidence:
- Static analysis of
git diff origin/master...HEAD - shell syntax and POM XML checks passed
git diff --checkreported whitespace-only errors
- Static analysis of
apache#3081 - Improved review comments. - Synced METADATA, CURRENT AND OPTIONS to cloud so that we can recover the DB after cluster crash scenarios. - Improved the resiliancy and reduced the data loss possibilities.
apache#3081 - Implemented review comments, Delete or tombstone the remote database generation during database destruction and make hydration honor that generation marker. Updated the runbooks and documentation for this. - Fixed the test coverage issues reported by previous run
apache#3081 - Implemented review comment Cloud upload blocks the RocksDB event thread, S3 service failures bypass the provider retry contract,Multipart wrapping erases the direct-DLQ marker,Release metadata lists different dependency versions, improved test reporting.
apache#3081 - Added few missing configs in hugegraph-store/hg-store-dist/src/assembly/static/conf/application.yml
apache#3081 - Fixed UT failures and code coverage issues.
VGalaxies
left a comment
VGalaxies
left a comment
There was a problem hiding this comment.
🚨 Review summary
Important
The cloud recovery path has blocking consistency, configuration, deletion-safety, release-metadata, and verification gaps.
📊 Risk dashboard
| Signal | Result |
|---|---|
| 🚦 Review gate | Blocked |
| 🔎 Actionable findings | 9 (8 High, 1 Medium) |
| 🧪 Verification coverage | 5 checks |
| 2 / 1 |
🔬 Coverage details
⚠️ S3 failure semantics — Confirmed
Trace: S3CloudStorageProvider upload, download, delete, and exception-classification paths, CloudStorageEventListener purge and hydration flows, AWS SDK 2.33.8 retry-code and file-transformer behavior
Conclusion: Confirmed ignored per-object deletion errors, retryable error-code misclassification, and non-atomic download recovery risks; lower-impact lifecycle leads were dropped.
⚠️ Release and E2E integrity — Confirmed
Trace: install-dist release LICENSE and known-dependencies inventory, cloud-storage Docker artifact build and recovery workflow, Maven provider packaging and test activation
Conclusion: Confirmed the release dependency inventory mismatch and E2E false-green paths caused by suppressed build and wipe failures.
🟡 Focused unit suites — Limited
Trace: hg-store-common cloud configuration, provider, and factory tests, hg-store-cloud-s3 exception-classification tests, hg-store-node listener, retry, metrics, tracker, configuration, and callback tests, hg-store-core BusinessHandlerImplTest
Conclusion: Surefire reports recorded 184 tests with 0 failures, 0 errors, and 0 skipped. The suites do not cover concurrent metadata publication, comma-separated data roots, or partial S3 batch deletion.
✅ Maven reactor — Clear
Trace: Full 44-project reactor with the cloud-s3 profile, hugegraph-store module ordering and dependency graph
Conclusion: Maven validate completed successfully for all 44 reactor projects; the suspected duplicate cloud-s3 module activation did not reproduce.
✅ Shell syntax — Clear
Trace: pd-entrypoint.sh, store-entrypoint.sh, test-graph-queries-and-sst.sh
Conclusion: bash -n returned successfully for all three changed shell entrypoints and workflows.
Warning
Verification limits
- No live Docker and MinIO recovery drill was run, so cluster recovery and purge behavior were not exercised end to end.
- The test environment used Java 17; the legacy JaCoCo agent emitted unsupported class-version instrumentation errors, so coverage instrumentation was not verified even though test assertions passed.
- S3SingleLargeFileE2ETest was intentionally not run because its default path generates a 20 GiB file.
- The independent recovery-consistency run timed out after 900 seconds and produced no usable result.
🤖 Codex review · GPT-5.6 Sol · effort: xhigh
| * | ||
| * <p>Package-private for testability. | ||
| */ | ||
| boolean syncMetadataSnapshotInline(CloudStorageProvider provider, String dbName) { |
There was a problem hiding this comment.
🚨 High · Serialize metadata publication per database
🧪 Reproduction path
- Prepare two snapshots for one database whose manifests represent successive generations.
- Block the older publication after capture.
- Allow the newer snapshot to publish CURRENT and prune metadata.
- Release the older publication and observe it overwrite CURRENT and prune the newer manifest.
Two upload-dispatch threads and the RocksDB compaction callback can concurrently enter syncMetadataSnapshotInline() for the same database. There is no per-database lock or generation check, while every invocation overwrites the same CURRENT key and then prunes metadata not belonging to its own snapshot.
Important
Impact: A slower, older snapshot can publish after a newer snapshot and delete the newer manifest. Disaster recovery can therefore roll back to stale data or find CURRENT pointing at metadata that another publication removed.
🛠️ Suggested change
Serialize capture, publication, and pruning per database, and reject snapshots older than the last published RocksDB generation. Add a deterministic concurrency test with deliberately reordered uploads.
🤖 Codex review · GPT-5.6 Sol · effort: xhigh
| try { | ||
| CloudStorageProviderFactory.initialize(cfg); | ||
| String resolvedDataRoot = | ||
| Paths.get(dataPath).toAbsolutePath().normalize().toString(); |
There was a problem hiding this comment.
🚨 High · Support every configured HStore data root
🔄 Contract mismatch
| Expected behavior | Actual behavior |
|---|---|
| Each configured data directory maps to stable relative cloud keys and restores files to that same directory. | All configured paths are interpreted as one literal filesystem path containing commas. |
HStore supports comma-separated app.data-path values and PartitionManager explicitly splits them. Cloud initialization instead passes the entire comma-separated string to Paths.get() and constructs one CloudStorageEventListener data root.
Important
Impact: For multi-directory stores, SST paths do not match the synthetic combined root. Upload keys fall back to container-specific absolute paths, hydration resolves them beneath a nonexistent comma-named directory, and the DLQ is persisted in the wrong location.
🛠️ Suggested change
Parse every configured data root and resolve each database or SST against its actual matching root. Preserve that root association when constructing keys and hydration destinations.
🤖 Codex review · GPT-5.6 Sol · effort: xhigh
| cfg.getUploadRetryMaxAttempts(), | ||
| cfg.getUploadRetryInitialDelayMs(), | ||
| cfg.getUploadRetryMaxDelayMs()); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
🚨 High · Do not silently disable explicitly enabled cloud storage
🔄 Contract mismatch
| Expected behavior | Actual behavior |
|---|---|
| An invalid enabled cloud-storage configuration prevents the node from advertising healthy service. | Initialization errors are logged and startup continues with cloud callbacks absent. |
The catch-all around provider initialization, retry-queue creation, and listener registration logs every exception and lets PostConstruct complete. An unknown provider, missing S3 bucket, or later setup failure therefore leaves the node running without an active listener.
Important
Impact: Operators can enable cloud recovery and receive normal service while no SST or metadata is mirrored. The durability failure remains invisible until local storage is lost.
🛠️ Suggested change
Fail startup or mark the node unhealthy when enabled cloud storage cannot initialize. If degraded local-only operation is required, make it an explicit configuration policy and expose its state through health checks and metrics.
🤖 Codex review · GPT-5.6 Sol · effort: xhigh
| .objects(toDelete) | ||
| .build()) | ||
| .build()); | ||
| totalDeleted += deleteResp.deleted().size(); |
There was a problem hiding this comment.
🚨 High · Treat partial DeleteObjects responses as purge failures
🔄 Contract mismatch
| Expected behavior | Actual behavior |
|---|---|
| A database purge succeeds only after all stale objects are deleted while the tombstone remains protective. | Partial deletion is reported as success and may remove the tombstone before stale objects. |
deletePrefix() counts deleteResp.deleted() but never inspects deleteResp.errors(). S3 can return HTTP success containing per-key failures, so the fallback runs only when the entire request throws. Database deletion also places the _DELETED tombstone inside the same prefix being purged.
Important
Impact: The tombstone can be deleted successfully while stale SST or metadata keys remain. A later database open then sees no deletion marker and can hydrate data from the deleted generation.
🛠️ Suggested change
Inspect and retry every per-object error, report any keys that remain, and verify the prefix is empty. Preserve the tombstone until successful verification, preferably in a generation namespace outside the data being purged.
🤖 Codex review · GPT-5.6 Sol · effort: xhigh
| return new IOException("S3 " + operation + " failed for key='" + key + "'", e); | ||
| } | ||
|
|
||
| private static boolean isRetryableServiceFailure(AwsServiceException e) { |
There was a problem hiding this comment.
⚠️ Medium · Honor AWS retryable error codes
🔄 Contract mismatch
| Expected behavior | Actual behavior |
|---|---|
| Errors classified as retryable by the AWS SDK remain eligible for provider and whole-file retries. | Retryable error codes with non-listed HTTP statuses are treated as permanent. |
isRetryableServiceFailure() checks throttling and selected HTTP statuses only. AWS SDK 2.33.8 explicitly marks PriorRequestNotComplete, RequestTimeout, RequestTimeoutException, and InternalError as retryable, but an HTTP 400 RequestTimeout is classified here as CloudStorageNonRetryableException.
Warning
Impact: Transient S3 failures bypass the configured whole-file retry policy and move an SST directly toward the DLQ, leaving it local-only until another compaction happens to retry it.
🛠️ Suggested change
Use the AWS SDK retry-condition logic or include its retryable error-code set, with focused HTTP 400 RequestTimeout and PriorRequestNotComplete tests.
🤖 Codex review · GPT-5.6 Sol · effort: xhigh
| Path destinationPath = Paths.get(localPath); | ||
| try { | ||
|
|
||
| s3Client.getObject( |
There was a problem hiding this comment.
🚨 High · Publish downloaded files atomically
🔄 Contract mismatch
| Expected behavior | Actual behavior |
|---|---|
| Only complete downloads become visible at RocksDB paths, and retries replace stale temporary files safely. | Downloads expose destination files before completion and existing partial artifacts block or bypass recovery. |
The S3 client writes directly to the supplied destination. Startup hydration skips any destination that already exists, while read-miss hydration reuses a fixed .hydrate path. AWS SDK ResponseTransformer.toFile does not replace an existing destination.
Important
Impact: A process termination or cleanup failure during download can leave a partial final SST that startup treats as complete, or a stale .hydrate file that permanently blocks retries. RocksDB recovery then fails or opens against corrupted data.
🛠️ Suggested change
Always download to a unique sibling temporary file, validate successful completion, and atomically replace the destination. Remove stale temporary artifacts on failure and startup.
🤖 Codex review · GPT-5.6 Sol · effort: xhigh
| https://central.sonatype.com/artifact/io.netty/netty-all/4.1.42.Final -> Apache 2.0 | ||
| https://central.sonatype.com/artifact/io.netty/netty-all/4.1.44.Final -> Apache 2.0 | ||
| https://central.sonatype.com/artifact/io.netty/netty-all/4.1.61.Final -> Apache 2.0 | ||
| https://central.sonatype.com/artifact/io.netty/netty-buffer/4.1.108.Final -> Apache 2.0 |
There was a problem hiding this comment.
🚨 High · Regenerate the release dependency inventory
📐 Measured result
| Metric | Observed | Required / target |
|---|---|---|
| bundled artifacts absent at their resolved version | 17 | 0 |
| incorrect Netty 4.1.108 entries | 10 | 0 |
known-dependencies.txt adds ten Netty 4.1.126.Final artifacts, while LICENSE adds their 4.1.108.Final versions. LICENSE also omits commons-codec-1.17.1, http-auth-aws-eventstream-2.33.8, httpcore-4.4.16, retries-2.33.8, retries-spi-2.33.8, slf4j-api-1.7.36, and url-connection-client-2.33.8.
Important
Impact: The Apache binary-release LICENSE describes dependencies that are not bundled and omits dependencies that are bundled, blocking an accurate release.
🛠️ Suggested change
Regenerate LICENSE from the resolved distribution, replace the ten incorrect Netty versions, add every omitted artifact, and compare the packaged JAR inventory against the release documents.
🤖 Codex review · GPT-5.6 Sol · effort: xhigh
| YAML | ||
| prepare_artifacts | ||
| log "building local cloud-storage images..." | ||
| docker compose -f "$COMPOSE_FILE" build --no-cache 2>&1 | grep -E "^(Building|FINISHED|\[|Successfully|Error)" || true |
There was a problem hiding this comment.
🚨 High · Abort the E2E workflow when image builds fail
🔄 Contract mismatch
| Expected behavior | Actual behavior |
|---|---|
| Any PD or Store image build failure stops the workflow before containers start. | Every build failure is converted to success and cached images may be reused. |
The script enables pipefail but appends || true to docker compose build. It then starts services without --build, so cached cloud-storage-local images remain eligible for use.
Important
Impact: The workflow can pass against stale images after the pull request code failed to compile or package, producing a false recovery result.
🛠️ Suggested change
Preserve and check the compose build exit status while filtering output, abort before compose up on failure, and record the image IDs used by the test.
🤖 Codex review · GPT-5.6 Sol · effort: xhigh
| local idx="$1" | ||
| local vol="${COMPOSE_PROJECT_NAME}_hg-store${idx}-data" | ||
| docker compose -f "$COMPOSE_FILE" stop "store${idx}" >/dev/null 2>&1 || true | ||
| docker run --rm --entrypoint /bin/sh -v "${vol}:/s" "$MINIO_MC_IMAGE" -c ' |
There was a problem hiding this comment.
🚨 High · Verify that the recovery test actually wipes local state
🔄 Contract mismatch
| Expected behavior | Actual behavior |
|---|---|
| Recovery verification proceeds only after the active Store volume's RocksDB state was demonstrably removed. | Destructive setup failures are ignored, so unchanged local data can satisfy the recovery assertion. |
Stopping the store, mounting and deleting the volume contents, and restarting the store all ignore failures. A wrong volume name can create and wipe a new empty volume while the real Store volume and its data remain untouched.
Important
Impact: The before-and-after vertex counts can match because the original local RocksDB data was never removed, allowing the recovery test to pass without reading anything from cloud storage.
🛠️ Suggested change
Fail closed on stop, volume mount, deletion, and restart errors. Assert that expected non-Raft directories existed before deletion and are absent before restarting the Store.
🤖 Codex review · GPT-5.6 Sol · effort: xhigh
2 participants
Purpose
This PR introduces a cloud storage architecture for HugeGraph Store (HStore). In cloud-native environments, storage nodes are ephemeral, making reliance on local disk storage a single point of failure. This implementation decouples the storage layer from local disk dependencies, allowing SST files to be offloaded to durable, scalable cloud storage providers.
Key Changes
Pluggable Architecture
CloudStorageProviderSPI interface to enable extensible storage backends.CloudStorageProviderFactoryfor seamless provider discovery and lifecycle management.S3 Provider Implementation
hg-store-cloud-s3module, leveraging AWS SDK v2 for production-ready S3/S3 compatible storage interaction.Lifecycle & Event Integration
CloudStorageEventListenerwithRocksDBFactory. This hooks into critical SST lifecycle events (onTableFileCreated,onTableFileDeleted) to ensure synchronized state between local RocksDB and cloud storage.onDBCreated) to backfill pre-existing files and read-miss on-demand hydration to ensure data availability.Infrastructure & Testing
docker/cloud-storage/using MinIO.test-graph-queries-and-sst.sh) to verify end-to-end data durability and query consistency.Implementation Highlights
META-INF/services.RocksdbEventListenerwithinRocksDBFactoryto intercept file operations without modifying core RocksDB logic.onReadMissto prevent redundant hydration requests during high-concurrency read scenarios.Verifying These Changes
CloudStorageConfigTest), factory registration, and event listener logic.docker-composesetup with MinIO.application.ymlbindings for credentials, bucket management, and sync intervals.Impact
LICENSEandNOTICEaccordingly.cloud.storagenamespace toapplication.yml(disabled by default).Documentation
hugegraph-store/docs/pluggable-cloud-storage-architecture.md.