Repository navigation
Conversation
…age with S3 support out of the box
dosubot
Bot
added
size:XXL
apache#3081 -added multipart upload for the S3 storage provider
6a3f496 to
8b6d597
Compare
apache#3081 -added multipart upload for the S3 storage provider
8b6d597 to
4275426
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3081 +/- ##
============================================
+ Coverage 55.93% 56.39% +0.45%
- Complexity 2277 6593 +4316
============================================
Files 774 772 -2
Lines 68534 69034 +500
Branches 9333 9837 +504
============================================
+ Hits 38334 38930 +596
+ Misses 26634 26531 -103
- Partials 3566 3573 +7
☔ 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
apache#3081 - Implemented review comments.
Hi @VGalaxies. |
apache#3081 - Improved code coverage.
apache#3081 - Improved code coverage.
|
@VGalaxies Not sure why the coverage is suddenly dropped at project level to 30.60% (-11.72%) compared to e960cc5 codecov/projectFailing after 1s — 30.60% (-11.72%) compared to e960cc5 |
The provider is resolved once in AppConfig and never reconfigured, so holding it in the static CloudStorageProviderFactory only added unreachable null-provider branches and a second retry budget. - Resolve the provider via ServiceLoader in AppConfig, init it once and pass it to CloudUploadRetryQueue and CloudStorageEventListener as a final, non-null field; AppConfig owns closing it. - Delete CloudStorageProviderFactory and its tests, the provider == null branches, the provider-unavailable retry budget and the *ForTest hooks. - Fix the architecture doc: there is no plugins/ directory scan. - Migrate hg-store-node's JUnit 4 tests to JUnit 5. Surefire only ran the Jupiter engine there, so they were silently skipped. - Run all hg-store-node tests in the store CI job, not just StoredRowIngressTest.
apache#3081 -Fix CI Errors due to Rockdbjni
apache#3081 -Fix Code Coverage issue
apache#3081 -Fix Code Coverage issue
apache#3081 -Address review comments.
apache#3081 - Fixed code coverage
-Exclude Lombok-generated code from JaCoCo coverage via hugegraph-store/lombok.config (lombok.addLombokGeneratedAnnotation) -Drop coverage-only equals/hashCode/canEqual branch tests from CloudStorageConfigTest -Exclude credentials from toString(): CloudStorageConfig.providerProperties and S3CloudStorageConfig accessKey/secretKey, with tests
- Minor fixes to improve code coverage
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The biggest simplification left is the metadata-sync debouncer in CloudStorageEventListener: a dirty set drained by scheduleWithFixedDelay on the scheduler the listener already owns replaces the leading/trailing edge logic, the CAS-drained count bound, three per-DB maps and the metadata-sync-max-unpublished setting. Second, crtBuilder() pulls in the 19 MB native aws-crt jar only for uploads above 512 MB, which the Java async client with multipartEnabled(true) covers with dependencies already on the classpath. Evidence: read the full diff at e8f494d (gh pr diff 3081 -R apache/hugegraph, 75 files, +20864/-242) and the touched files at that head in a detached worktree; grep across hugegraph-store/*/src/main for notifyTruncateBegin, requestDebouncedMetadataSync, UrlConnectionHttpClient, crtBuilder and implements CloudStorageProvider; checked the s3 2.33.8 POM (runtime apache-client and netty-nio-client) and the aws-crt-0.38.9.jar size on Maven Central. The five findings from the previous simplification round (provider factory, DLQ persistence, static executors, duplicated config bean, duplicated marker code) are addressed at this head and are not repeated.
337ed3b to
e8f494d
Compare
# Conflicts: # .github/workflows/pd-store-ci.yml # hugegraph-store/hg-store-dist/src/assembly/static/conf/application.yml # hugegraph-store/hg-store-node/src/main/resources/application.yml # install-dist/scripts/dependency/known-dependencies.txt
14fc430 to
ec9a692
Compare
Addressed review comments -Replaced the hand-built metadata-sync debouncer with a dirty-DB set drained by scheduleWithFixedDelay on the existing scheduler. Failed publishes retry on the next run, truncating DBs are skipped, and close() flushes dirty DBs once. Removed the leading/trailing-edge logic, the CAS counter, three per-DB maps and the metadata-sync-max-unpublished setting. -Built the async S3 client with S3AsyncClient.builder() .multipartEnabled(true) instead of crtBuilder(), and apply buildRetryOverride() to both clients. Dropped aws-crt-client and url-connection-client, and updated known-dependencies.txt and LICENSE. -Made CloudStorageProvider.listFiles() and deletePrefix() abstract so a provider cannot silently inherit an empty listing or a no-op purge. Removed CloudStorageProviderTest. -Simplified isActivelyTruncating() to a set lookup. RocksDBSession.truncate() now always clears the latch in a finally, so removed the stale-latch timeout, its test hook and the self-healing test. -Extracted downloadAtomically(), moveReplacing() and writeTombstone(). Used the .hyd-tmp-* temp name everywhere so the startup sweep also removes read-miss restore leftovers, and fixed the Docker test script that searched for the wrong temp name.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Score: 0/10. Summary: The cloud storage design is well documented, and the latest commit addresses the earlier simplification comments (dirty-set drain, Java multipart client, abstract listFiles/deletePrefix, truncate latch in finally, shared download/tombstone helpers). Two purge-retry paths can delete live cloud data for a re-created or truncated DB, and the native RocksDB listener is now attached for every store node, which changes compaction behavior (a Raft blank task per compaction), logging and the launch command for users who never enable cloud storage. Several restore paths (versioned directories, dropped dirty marks during truncation) can leave the cloud copy unrestorable, and some new tests and E2E checks cannot fail or do not run in CI. Evidence: Read the full diff at ede0270 with the callers in hg-store-core and hg-store-rocksdb. On JDK 17 ran the hg-store-common, hg-store-rocksdb, hg-store-core, hg-store-node and hg-store-cloud-s3 unit tests for the changed classes: 306 tests, 0 failures, 1 skipped (the directory-permission test skips when run as root). Ran mvn dependency:tree for hg-store-cloud-s3 and for hg-store-node with -Ps3-cloud-storage: known-dependencies.txt and LICENSE match the runtime tree. Reproduced the duplicate-key binding failure with Spring Boot 2.5.15 Binder. Did not run the Docker E2E scripts or S3SingleLargeFileE2ETest, and did not reproduce the checksum issue against a non-AWS endpoint. Docs: the PR adds user-visible configuration (cloud.storage.*), a build profile and launcher changes; please link the paired apache/hugegraph-doc PR for the Store configuration and deployment pages if one exists.
|
The earlier review comments below still apply at 084ae60; this head does not fix them.
|
…apache#3081 - Guard pending-delete and truncate purge retries with per-prefix/per-DB epochs so a retry never purges a re-created or re-written DB; block the open when pending-delete cleanup cannot complete - Attach the native RocksDB listener only when a cloud listener is registered, and restore the EnabledEventCallback mask - Publish metadata on compaction via the metadata-sync thread instead of inline; back off failed publishes per DB and dedupe retry-queue entries - Keep dirty marks during truncate/grace; flush reliably on close() - Make the destroy guard path-aware, capture checkpoints under the session lock with a held reference, sweep stale _cloudmeta_ and .hyd-tmp- files, record logical DB names before open - Discard creations that finish after releaseAllGraphDB(); bound waits on in-flight creations with per-DB futures - S3: checksums WHEN_REQUIRED, default region for custom endpoints, bind provider properties only when enabled, stage only the SDK closure - CI runs S3CloudStorageProviderTest - Docker harness: Gremlin vertex counts, real tombstone assertions, HStore backend in the static compose; RAT/editorconfig cover tracked files - Docs: metric names, plugin setup, behaviour changes and known limitation for version-suffixed directories; fix NOTICE separators
- Pin pgsty/minio to RELEASE.2026-08-04T00-00-00Z and pgsty/mc to RELEASE.2026-10-18T00-00-00Z in the static compose, generated compose, test scripts and README (the digests :latest resolves to today) - Document that a destroyed RocksDB with live references is now force-released after 30 minutes instead of immediately (deleted within ~1-2 min before), on all nodes with or without cloud storage
Address remaining review comments. - Keep the pending-delete marker until both the prefix purge and the tombstone delete succeed, so a leftover tombstone can no longer purge a re-created DB - Resolve a leftover tombstone on every open, also with startup hydration disabled, and clear the sync tracker after every successful purge - Make onDBDeleted path-aware: deleting a stale directory keeps the state of the same DB live at another directory - Queue superseded-SST deletes and run the live-set check, metadata publish and remote delete on the metadata-sync thread, one publish per DB per pass, under the per-DB publish backoff - Publish dirty metadata from a new onDBClosing hook while the session is still open; shutdown released every DB before the listener closed, so the final publish never happened - Open a DB with no local CURRENT at the newest restorable remote version-suffixed sibling, so a partition in <name>_<v> is restored after disk loss; listFiles is documented as a plain key-prefix match - Let an env var win over the same provider key from application.yml instead of failing startup with a duplicate key - Drop the read-miss hook on the null-result path, which cannot change a genuine key-not-found - Docker README: target the cloud-storage-test stack, use /versions for health, pass --compressed to curl commands whose output is parsed; remove the dead tombstone SKIPPED branch from the E2E script - Docs updated for all behaviour changes
Thanks @bitflicker64 for the thorough review. All items in your list are addressed. Can you please take a look? |
…-Fix flaky publish-count UT onTableFileDeleted_runsOffCallerThread_withOnePublishPerDbPerPass flipped metadata to healthy before any publish had failed, so a pass that saw only the first queued delete could publish on its own and a second pass published again. Wait for the first failed publish so its backoff holds later passes until all deletes share one publish.
…-Raise publish-count UT wait timeout Add a waitForCondition overload with an explicit timeout and give onTableFileDeleted_runsOffCallerThread_withOnePublishPerDbPerPass 10 s, since its healthy publish must wait out the >= 1 s publish backoff and 2 s left little headroom on slow CI.
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.Conceptual overview
Concept illustration of the proposed HStore cloud-storage SPI, S3 provider, upload and metadata synchronization, and hydration paths. Not full review or runtime evidence.