Repository navigation
Conversation
…age with S3 support out of the box
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3081 +/- ##
============================================
- Coverage 55.93% 55.13% -0.81%
- Complexity 2277 2318 +41
============================================
Files 774 771 -3
Lines 68534 68596 +62
Branches 9333 9469 +136
============================================
- Hits 38334 37819 -515
- Misses 26634 27273 +639
+ Partials 3566 3504 -62
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
apache#3081 - Improved test coverage.
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
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 -Added back the dependency went missing due to merge conflicts
apache#3081 - Implemented checkstyle fixes and also other CI failures
1528cd0 to
1cf07b0
Compare
# Conflicts: # hugegraph-store/hg-store-core/src/main/java/org/apache/hugegraph/store/metric/SystemMetricService.java # hugegraph-store/hg-store-dist/src/assembly/descriptor/server-assembly.xml # hugegraph-store/hg-store-dist/src/assembly/static/bin/start-hugegraph-store.sh # hugegraph-store/hg-store-rocksdb/pom.xml # hugegraph-store/pom.xml # install-dist/scripts/dependency/known-dependencies.txt
Master's shared-foundation check discovers every hugegraph-store/*/pom.xml module, so gating hg-store-cloud-s3 behind the s3-cloud-storage profile made the check fail outside that profile. Build the module by default; the hg-store-node profile still decides whether the AWS SDK ships in the fat jar. Register the newly resolved runtime dependencies (AWS SDK for Java 2.33.8, aws-crt, eventstream, commons-codec 1.17.1, httpcore 4.4.16, slf4j-api 1.7.36, reactive-streams 1.0.4) in known-dependencies.txt, LICENSE and NOTICE. Add per-component license files only for the non-Apache-2.0 text: slf4j-api (MIT), reactive-streams (MIT-0) and third-party-jackson-core (bundled FastDoubleParser and bigint). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
apache#3081 - Fix the docker image issues after the JDK 17 upgrade.
apache#3081 - Fix the docker image issues after the JDK 17 upgrade.
8c00910 to
bafbeae
Compare
The store launcher now requires one rocksdbjni jar on the classpath for native callback classloading, so the fixture must provide a placeholder. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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.
| * immediately (delete guard, {@link #onCompacted}, {@link #onDBCreated}) call | ||
| * {@link #syncMetadataSnapshotInline} directly and are intentionally not routed through here. | ||
| */ | ||
| void requestDebouncedMetadataSync(CloudStorageProvider provider, String dbName) { |
There was a problem hiding this comment.
Important: replace or collapse. This is a hand-built debouncer: a leading-edge publish, a trailing-edge reschedule, a count bound drained by a floor-at-zero CAS loop, three per-DB maps (lastMetadataSyncMs, unpublishedUploads, pendingMetadataSync), a second knob (metadata-sync-max-unpublished in CloudStorageConfig and both application.yml files), and a drain loop in close(). A dirty set drained by the scheduler the listener already owns keeps the same contract (at most one publish per window per DB, always published eventually):
private final Set<String> dirtyDbs = ConcurrentHashMap.newKeySet();
// constructor, when metadataSyncDebounceMs > 0
metadataSyncScheduler.scheduleWithFixedDelay(this::publishDirtyDbs, metadataSyncDebounceMs,
metadataSyncDebounceMs, TimeUnit.MILLISECONDS);
void requestDebouncedMetadataSync(CloudStorageProvider provider, String dbName) {
if (isActivelyTruncating(dbName) || isInTruncationGracePeriod(dbName)) {
return;
}
if (metadataSyncDebounceMs <= 0) {
syncMetadataSnapshotInline(provider, dbName);
} else {
dirtyDbs.add(dbName);
}
}
private void publishDirtyDbs() {
for (String db : dirtyDbs) {
dirtyDbs.remove(db);
try {
if (!isActivelyTruncating(db) && !isInTruncationGracePeriod(db)
&& !syncMetadataSnapshotInline(provider, db)) {
dirtyDbs.add(db); // failed publish retries on the next tick
}
} catch (RuntimeException e) {
dirtyDbs.add(db);
log.warn("Deferred cloud metadata-sync failed for db={}: {}", db, e.getMessage());
}
}
}close() calls publishDirtyDbs() once after shutdownNow(). Publishing moves off the upload threads, which is the starvation the CAS comment works around, and a failed publish is retried instead of waiting for the next upload. That deletes runTrailingMetadataSync, the CAS loop, the three maps, the counter reset in syncMetadataSnapshotInline, and the max-unpublished setting. The fixed delay already bounds how far the mirrored manifest can trail the uploads.
| .build()); | ||
| } | ||
|
|
||
| S3CrtAsyncClientBuilder asyncBuilder = S3AsyncClient.crtBuilder(); |
There was a problem hiding this comment.
Important: replace or collapse. crtBuilder() is the only reason aws-crt-client and the native aws-crt-0.38.9.jar (19,062,529 bytes on Maven Central) ship, and it only serves files above MULTIPART_THRESHOLD_BYTES (512 MB). The s3 artifact already brings netty-nio-client at runtime, and the SDK's Java async client does multipart on its own:
S3AsyncClientBuilder asyncBuilder = S3AsyncClient.builder()
.multipartEnabled(true)
.overrideConfiguration(buildRetryOverride());The credentials, region, endpoint and forcePathStyle(true) lines below stay as they are, and S3TransferManager accepts this client, so uploadViaTransferManager does not change. Then drop aws-crt-client from hg-store-cloud-s3/pom.xml. url-connection-client can go in the same edit: nothing references UrlConnectionHttpClient, and s3 already brings apache-client for the sync client. That takes aws-crt, aws-crt-client and url-connection-client out of known-dependencies.txt, LICENSE and NOTICE.
While there: the javadoc on buildRetryOverride() says it applies to both clients, but only the sync builder gets it (L155). The snippet above fixes that as well.
| * @return object keys relative to provider root (without provider pathPrefix) | ||
| * @throws IOException on I/O or network failure | ||
| */ | ||
| default List<String> listFiles(String remoteDirPrefix) throws IOException { |
There was a problem hiding this comment.
Important: should not exist. This interface is new in this PR, so there are no "existing providers" to stay compatible with, and the one real provider, S3CloudStorageProvider, overrides both listFiles and deletePrefix. These defaults are also the unsafe kind. A provider that forgets listFiles gets an empty listing, so startup hydration finds nothing. The default deletePrefix then deletes nothing and returns 0, which purgeRemotePrefix reads as success, and onDBDeleted goes on to remove the tombstone and the local marker.
Make both abstract:
List<String> listFiles(String remoteDirPrefix) throws IOException;
int deletePrefix(String remoteDirPrefix) throws IOException;That deletes the two default bodies and CloudStorageProviderTest (359 lines), which only tests these defaults. Any test double that does not override them gets a one-line stub.
| * lifetime of the process. To bound that blast radius, treat the latch as stale (and clear it) | ||
| * once the recorded truncation start is older than {@link #MAX_TRUNCATION_DURATION_MS}. | ||
| */ | ||
| boolean isActivelyTruncating(String dbName) { |
There was a problem hiding this comment.
Important: should not exist. The javadoc above says RocksDBSession.truncate() runs dropTables/createTables "without a finally", so the latch can stick. That was true before this PR. This PR's own truncate() (RocksDBSession.java L664-693) now always ends in notifyTruncate or notifyTruncateAbort from a finally; it is the only caller of notifyTruncateBegin; and onDBTruncated clears the flag in its own finally. truncateAbort_clearsLatchImmediately_withoutPurgingRemote already covers that path. The stale-latch timeout has nothing left to recover from:
boolean isActivelyTruncating(String dbName) {
return truncatingDbs.contains(dbName);
}Then delete MAX_TRUNCATION_DURATION_MS, maxTruncationDurationMs, setMaxTruncationDurationMsForTest() and truncatingLatch_selfHealsWhenTruncationNeverCompletes in CloudStorageIntegrationTest, whose setup comment still describes the code before the finally.
| // makes a late second mover a harmless idempotent overwrite. A crash mid-download | ||
| // never leaves RocksDB reading a truncated SST at the expected path. | ||
| Path tmp = localPath.resolveSibling( | ||
| localPath.getFileName() + ".hydrate-" + Thread.currentThread().getId() |
There was a problem hiding this comment.
Minor: local tidy. Download to a temp file, then atomic move with the AtomicMoveNotSupportedException fallback, is written out here and again in preHydrateDbFiles (L2315-2332), and the move-with-fallback a third time in reconcileCurrentPointer (L2418-2425). The two download copies already disagree on the temp name: this one writes .hydrate-<tid>-<ns>, but the startup sweep at L2285 only removes .hyd-tmp*, so a crashed read-miss restore leaves its temp file behind for good. One helper, one name:
private void downloadAtomically(String remoteKey, Path dest) throws IOException {
Path tmp = dest.resolveSibling(dest.getFileName() + ".hyd-tmp-"
+ Thread.currentThread().getId() + "-" + System.nanoTime());
try {
provider.downloadFile(remoteKey, tmp.toString());
try {
Files.move(tmp, dest, ATOMIC_MOVE, REPLACE_EXISTING);
} catch (AtomicMoveNotSupportedException e) {
Files.move(tmp, dest, REPLACE_EXISTING);
}
} finally {
Files.deleteIfExists(tmp);
}
}The "legacy" bare .hyd-tmp match at L2285 can go too, since no build with that name ever shipped. The tombstone upload is the same story: onDBDeleteBegin (L651-669) and ensureTombstonePresent (L1041-1062) each spell out the temp-file write, upload and cleanup, and one writeTombstone(prefix) covers both.
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.
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.