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 58.42% 58.79% +0.37%
+ Complexity 6903 3129 -3774
============================================
Files 779 775 -4
Lines 69391 69531 +140
Branches 9567 9607 +40
============================================
+ Hits 40539 40882 +343
+ Misses 25179 24963 -216
- Partials 3673 3686 +13
*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:
|
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 |
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.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: Most earlier comments are fixed at a7a50c0, but an abandoned truncate purge can leave the cloud copy restoring cleared data, and getLiveSstFiles still reads a session it holds no reference to. The application.yml setup comment from the last round still applies.
The earlier review comments below still apply at a7a50c0; this head does not fix them.
- Minor: The setup instructions describe a route that does not work with the shipped launcher. (#3081 (comment))
…-Restore store-node test step lost in master merge The master merge took master's side of the pd-store-ci.yml conflict, which narrowed the store-node step to three named test classes. That dropped the cloud-storage tests from CI and broke the JaCoCo validator contract check, which still expects the org/apache/hugegraph/store/node/**/*Test glob. Restore the glob and -DreuseForks=false; the glob also covers master's HgStoreMetricsControllerTest.
…Address latest review comments - A truncate purge retry that finds the DB writing again no longer drops the marker: the remote CURRENT still names the cleared generation. It ends the grace window, marks the DB dirty, and the next successful publish reaps the remote SSTs that neither the published MANIFEST nor the live set references before removing the marker - Persist the pending reap in a reap marker so a restart reaps instead of purging the live prefix; reap offline (local SSTs as the keep-set) when this node does not reopen the DB, and publish then reap from onDBClosing - Retry a truncate purge whose marker could not be written; stand a startup retry down once the open has purged; resolve startup markers to the RocksDB session name (dbNameOfPrefix) - getLiveSstFiles holds a session reference (queryGraphDB, closed in finally) and reads the live files under the cf-handle read lock, so a release cannot close the DB mid-call - Queue superseded-SST deletes during truncation and the grace window instead of dropping them; processDeferredSstDeletes already holds them until the window ends - application.yml and hg-store-cloud-s3 pom: document the -Ps3-cloud-storage build as the supported route; a provider jar on the launch classpath cannot load - Architecture doc updated for the truncate stand-down and reap behaviour
| } | ||
| Object lock = pendingDeleteLocks.computeIfAbsent(prefix, k -> new Object()); | ||
| synchronized (lock) { | ||
| if (truncateReapPending.remove(prefix, dbName)) { |
There was a problem hiding this comment.
P2: Bind reap completion to the truncate generation, not just the DB name.
The new marker retention fixes the previous single-truncate path, but truncateReapPending.remove(prefix, dbName) can also match a later truncate's entry. A close-triggered inline publish/reap A can pause after its initial pending check. Another thread can truncate the same DB (B), clear A's entry, persist B's marker, fail the remote purge, and create a new SST. The scheduled retry then installs B's pending reap with the same prefix and DB name. When A resumes, this removal succeeds and deletes B's marker even though no post-B metadata has been published. With remote CURRENT still naming A's generation, subsequent local data loss and hydration can restore data cleared by B.
releaseGraphDB() invokes onDBClosing() before taking its operation write lock, and the truncate callbacks do not take the metadata-sync lock held by the inline reap, so the single-threaded metadata scheduler does not exclude this interleaving.
Please give each truncate/reap a generation token and remove its markers only when that token still matches, with synchronization covering the corresponding marker transition. Add a latch-controlled regression that pauses reap A, completes B's failed-purge/new-SST stand-down, then resumes A and verifies that B's marker remains until B's own successful publish and cleanup.
Evidence: static control-flow review of the exact version below plus a pure state-transition model; no Java/cloud concurrency reproduction or data-loss experiment was run.
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.