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 41.40% 41.66% +0.26%
- Complexity 7337 7433 +96
============================================
Files 802 806 +4
Lines 69792 70142 +350
Branches 9312 9361 +49
============================================
+ Hits 28897 29227 +330
- Misses 37604 37608 +4
- Partials 3291 3307 +16 ☔ 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 |
Please ignore. I was bit confused due to merge conflict I got in docker/README.md due to changes in 9aaad11. It seems only reference to JDK17 was removed from README.md. But we are not yet moved to JDK17. |
|
@imbajin @VGalaxies Tested following scenarios
|
apache#3081 - Improved the AML notebook.
apache#3081 - Improved the AML notebook.
apache#3081 - Set a stable node-id on all three store containers so the cloud key scope survives a full volume wipe. -Document the scope re-seed WARN in README and pluggable-cloud-storage-architecture.md, covering when it is safe and when it is a silent data-loss risk.
|
@VGalaxies @imbajin |
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The SPI/factory shape is out of scope here because issue #3079 explicitly asks for it, but the biggest simplification left is inside the S3 provider — init() sets no overrideConfiguration, so the AWS SDK is already retrying every uploadPart with jittered backoff, and the hand-rolled multipart driver plus its second retry layer (~400 of that file's 1100 lines) can go. Evidence: local diff of the exact head against the PR base (git diff 401e627c30a7a0e0db5548a9431b6acfe82e43a1 eddcd868cc97d574e75f0a5709899b0a4600b222, 62 files / +24239 / -84); read S3CloudStorageProvider.init() L176-248 and the multipart path L691-956, CloudStorageEventListener.applyBackpressure/dlqEnqueueRateBacklog L1858-1942 against upload-backpressure-high-watermark: 0 in both application.yml files, AppConfig.readProviderProperties() L723-737 against spring-boot 2.5.14 in hg-store-node/pom.xml, and the **/*.ipynb exemption added at .licenserc.yaml:69. Note: the GitHub Files API reports additions=0 for hg-store-rocksdb/** and pom.xml at this diff size — those files do carry changes; the local diff is authoritative. All four anchors verified against right-side added lines in the exact-head diff.
apache#3081 -Implemented review comments, removed multipart retry and leveraged s3-transfer-manager -Removed upload-backpressure-high-watermark - Replaced the manual AbstractEnvironment/EnumerablePropertySource streaming (~20 lines) with Binder.get(environment).bind("cloud.storage." + provider, Bindable.mapOf(...)).
apache#3081 -Fix docker-compose.dev.yml issue ( introduced due to merge conflicts)
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: the rework since the last round is real (the multipart driver, the inert backpressure path, the notebook and the property-source scan are all gone), and the biggest simplification left is the construction surface of CloudStorageEventListener: seven public constructors plus a Tuning/Builder pair plus equivalent setters, when production has exactly one caller using the full path, about 150 lines that one constructor with two extra parameters replaces. Second: CloudSyncTracker uses a Roaring64NavigableMap as a plain set, which costs a new RoaringBitmap dependency in hg-store-node and five synchronized blocks that ConcurrentHashMap.newKeySet() makes unnecessary. Evidence: reviewed the full diff at head f5b285d (60 files, +21633/-83) with the head fetched locally; git grep 'new CloudStorageEventListener(' shows one production call site (AppConfig) against 61 test call sites; CloudSyncTracker touches the bitmap only via addLong/removeLong/contains/getLongCardinality. The SPI/factory shape is not raised: issue #3079 explicitly asks for an SPI-based extensible architecture, and the retry queue's DLQ persistence is data-loss protection, not scaffolding.
- Implemented review comments 1.collapse CloudStorageEventListener/CloudUploadRetryQueue constructors 2.replace Roaring64NavigableMap with ConcurrentHashMap.newKeySet() in CloudSyncTracker
- Added back RoaringBitmap in known-dependencies.txt and LICENSE
-Added Missing file
RocksDB's onTableFileCreated/onTableFileDeleted/onCompactionCompleted callbacks run on RocksDB's own native threads with no Java caller frame, so they resolve classes via the plain system classloader. Inside a Spring Boot fat jar that classloader can't see classes nested under BOOT-INF/lib, so registering the listener always threw NoClassDefFoundError once the distribution jar was packaged. Add RocksDBFactory#hasRocksdbChangedListeners() and only call dbOptions.setListeners(...) in RocksDBSession#openRocksDB when at least one RocksdbChangedListener is actually registered, so the native callback path is never exercised unless a listener-based feature (e.g. cloud storage sync) is enabled.
- One more fix for the test failure in CI.
- Additional fix to address CU failure
- Fixed license issues. Now S3 provider is opt-in mechanism, so the known-dependencies.txt and LICENSE entries are reported as 3rd party check failures.
- It seems earlier the CI failure was because rockdb jni ws not on classpath. Updated Classpath a bit
- Fix redundant jsonassert-1.5.0.jar brought in due to merge commit.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The largest cut is the global mutable provider in CloudStorageProviderFactory. Passing the resolved provider into the listener and retry queue removes 16 null-provider branches and a second retry budget. The file-backed DLQ has no production reader because replayDlq() is only called from tests. Evidence: static reading of the full diff at 1cf07b0; grep for replayDlq, getActiveProvider, provider == null, metadataSyncScheduler() and uploadSubsystemShuttingDown across hg-store-*/src/main; RocksDBFactory.createGraphDB fires onDBCreated, which runs uploadExistingSstFiles on every open.
| * has not yet been called. | ||
| */ | ||
| @Getter | ||
| private static volatile CloudStorageProvider activeProvider; |
There was a problem hiding this comment.
Important: should not exist. The provider is set once in @PostConstruct and never reconfigured, yet it lives in a mutable global.
The listener and retry queue call getActiveProvider() at 18 sites with 16 provider == null branches. Those branches feed a second "provider unavailable" retry budget: DEFAULT_MAX_PROVIDER_UNAVAILABLE_RETRIES, computeProviderUnavailableDelay and two *ForTest setters in CloudUploadRetryQueue, plus TRUNCATE_PROVIDER_UNAVAILABLE_MAX_DELAY_MS in the listener. AppConfig returns before initialize when cloud storage is disabled, so the disabled branch here never runs. reset() and setActiveProviderForTest() are test hooks in main code.
Keep the SPI (#3079 asks for it). Resolve the provider once and pass it to both constructors as a final field:
CloudStorageProvider provider = ServiceLoader.load(CloudStorageProvider.class).stream()
.map(ServiceLoader.Provider::get)
.filter(p -> p.providerName().equals(cfg.getProvider()))
.findFirst()
.orElseThrow(() -> new IllegalArgumentException(
"No cloud storage provider '" + cfg.getProvider() + "' on the classpath"));
provider.init(cfg);That deletes the global, the null branches, the second retry budget and the test hooks.
| * fail again).</li> | ||
| * </ul> | ||
| */ | ||
| public void replayDlq() { |
There was a problem hiding this comment.
Important: should not exist. replayDlq() has no caller outside CloudUploadRetryQueueTest: no endpoint, no scheduled call, no startup call.
loadDlqFromDisk() reads .cloud-upload-dlq.tsv at startup and logs "call replayDlq() to retry them", but no operator path can do that. The loaded entries sit in memory unused. Recovery already happens without the file: createGraphDB fires onDBCreated on every open, and uploadExistingSstFiles uploads every live *.sst missing from the bucket. The PR's docs say evicted entries "stay recoverable via the delete guard and startup SST backfill".
That leaves about 380 lines writing a file nobody reads:
loadDlqFromDisk,appendDlqEntryToDisk,rewriteDlqFile,fsyncDirserialize/deserialize/escape/unescapedlqFileLock- the
dlqPersistenceHealthyflag, gauge andAppConfigbinding - this method
Requested change: keep the in-memory DLQ count for visibility and delete the persistence and replay. If replay is wanted, call replayDlq() once after loadDlqFromDisk() in the constructor so the file has a reader.
| * the same JVM binds new listeners to a live executor instead of a TERMINATED one (which would | ||
| * reject every upload via AbortPolicy and silently divert all SSTs to the DLQ). | ||
| */ | ||
| private static ThreadPoolExecutor sharedUploadExecutor; |
There was a problem hiding this comment.
Important: should not exist. Production builds one listener, but the upload pool and metadata-sync scheduler are static, with lazy re-creation in sharedUploadExecutor() / metadataSyncScheduler().
A static uploadSubsystemShuttingDown gate stops the scheduler from coming back during drain. Because the gate lets metadataSyncScheduler() return null, four call sites carry a scheduler == null branch (L899, L1165, L1796, L2114), and requestDebouncedMetadataSync checks the gate again. The stated reason is "a Spring context restart in the same JVM", which hg-store-node does not do.
Requested change: create both executors as private final instance fields in the constructor. Add a close() that calls uploadExecutor.shutdown(), awaitTermination(...) and then scheduler.shutdownNow(), and call it from AppConfig.onDestroy(). A schedule after shutdown throws RejectedExecutionException, which every call site already catches. The two static fields, both lazy getters, the gate and the four null branches go.
| @Data | ||
| @Configuration | ||
| @ConfigurationProperties(prefix = "cloud.storage") | ||
| public class CloudStorageSpringConfig { |
There was a problem hiding this comment.
Important: replace or collapse. This class re-declares all 12 fields of CloudStorageConfig with the same defaults, and toCloudStorageConfig() copies them one setter at a time. A new knob means three edits, and the two sets of defaults can drift.
CloudStorageConfig is already a Lombok @Data bean, so bind it directly:
CloudStorageConfig cfg = Binder.get(environment)
.bind("cloud.storage", CloudStorageConfig.class)
.orElseGet(CloudStorageConfig::new);
cfg.setProviderProperties(readProviderProperties(cfg.getProvider()));That deletes this inner @Configuration class and the copy method. Relaxed binding (CLOUD_STORAGE_ENABLED and so on) still works.
| * truncate-purge intent durable across process restarts while the provider is unavailable. | ||
| */ | ||
| @SuppressWarnings("ResultOfMethodCallIgnored") | ||
| private void writePendingTruncateMarker(String dbName, String prefix) throws IOException { |
There was a problem hiding this comment.
Important: replace or collapse. The delete and truncate marker code exists twice, and the copies differ only in the directory and the log text:
- This method matches
writePendingDeleteMarkerline for line (file fsync, directory fsync, Windows skip). The only extra there isdeleteMarkerHealthy = true. processPendingTruncateMarkersOnStartupmatchesprocessPendingDeleteMarkersOnStartupexcept for the log text and the final schedule call.pending*Dir,pending*MarkerPathandremovePending*Markercome in pairs.
Requested change: one writeMarker(Path dir, String prefix), one removeMarker(Path dir, String prefix) and one scanMarkers(Path dir, BiConsumer<String, String> onMarker) taking (dbName, prefix). Each caller passes its directory and callback, and the delete caller sets deleteMarkerHealthy itself. That removes about 90 lines.
While there: the file name is Base64(prefix), the body repeats the prefix, and the startup scan drops the marker unless the two match. Decode the name and write an empty file.
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.