Skip to content

feat(store): implement pluggable cloud storage for HStore - #3081

Open
vaijosh wants to merge 67 commits into
apache:masterfrom
vaijosh:Hstore+CloudStorage
Open

vaijosh wants to merge 67 commits into
apache:masterfrom
vaijosh:Hstore+CloudStorage

Conversation

@vaijosh

@vaijosh vaijosh commented Jul 6, 2026 •

Copy link
Copy Markdown
Contributor

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

  • Created the CloudStorageProvider SPI interface to enable extensible storage backends.
  • Added CloudStorageProviderFactory for seamless provider discovery and lifecycle management.

S3 Provider Implementation

  • Introduced the hg-store-cloud-s3 module, leveraging AWS SDK v2 for production-ready S3/S3 compatible storage interaction.

Lifecycle & Event Integration

  • Integrated CloudStorageEventListener with RocksDBFactory. This hooks into critical SST lifecycle events (onTableFileCreated, onTableFileDeleted) to ensure synchronized state between local RocksDB and cloud storage.
  • Implemented startup hydration (onDBCreated) to backfill pre-existing files and read-miss on-demand hydration to ensure data availability.

Infrastructure & Testing

  • Added a comprehensive local development environment via docker/cloud-storage/ using MinIO.
  • Included an integration test script (test-graph-queries-and-sst.sh) to verify end-to-end data durability and query consistency.

Implementation Highlights

  • SPI Integration: Standardized service discovery via META-INF/services.
  • RocksDB Hooking: Registered a singleton RocksdbEventListener within RocksDBFactory to intercept file operations without modifying core RocksDB logic.
  • Read-Miss Protection: Added a guard window mechanism in onReadMiss to prevent redundant hydration requests during high-concurrency read scenarios.

Verifying These Changes

  • Unit Tests: Coverage for config parsing (CloudStorageConfigTest), factory registration, and event listener logic.
  • Integration Tests: Verified using the new docker-compose setup with MinIO.
  • Configuration: Verified application.yml bindings for credentials, bucket management, and sync intervals.

Impact

  • Dependencies: Added AWS SDK v2; updated LICENSE and NOTICE accordingly.
  • Configurations: Added cloud.storage namespace to application.yml (disabled by default).
  • Public API: No breaking changes to existing public-facing client APIs.

Documentation

  • Architecture and configuration details are documented in hugegraph-store/docs/pluggable-cloud-storage-architecture.md.

Conceptual overview

Proposed HStore cloud storage event listener, provider SPI, S3 upload and metadata synchronization, and hydration paths

Concept illustration of the proposed HStore cloud-storage SPI, S3 provider, upload and metadata synchronization, and hydration paths. Not full review or runtime evidence.

@vaijosh
vaijosh marked this pull request as ready for review July 6, 2026 14:11
@dosubot dosubot Bot added size:XXL This PR changes 1000+ lines, ignoring generated files. feature New feature store Store module labels Jul 6, 2026
vaijosh added a commit to vaijosh/hugegraph that referenced this pull request Jul 8, 2026
apache#3081
-added multipart upload for the S3 storage provider
@vaijosh
vaijosh force-pushed the Hstore+CloudStorage branch from 6a3f496 to 8b6d597 Compare July 8, 2026 10:29
apache#3081
-added multipart upload for the S3 storage provider
@vaijosh
vaijosh force-pushed the Hstore+CloudStorage branch from 8b6d597 to 4275426 Compare July 8, 2026 10:59
@codecov

codecov Bot commented Jul 8, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.13%. Comparing base (c2ec570) to head (ede0270).
⚠️ Report is 1 commits behind head on master.

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     
Flag Coverage Δ
commons 96.37% <ø> (ø)
hstore 35.84% <ø> (+0.03%) ⬆️
pd 49.43% <ø> (+0.25%) ⬆️
server_hbase ?
server_memory 54.34% <ø> (+0.01%) ⬆️
server_rocksdb 42.31% <ø> (-0.03%) ⬇️
store 34.05% <100.00%> (+2.12%) ⬆️

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@VGalaxies VGalaxies left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 --check reported whitespace-only errors

Comment thread install-dist/release-docs/LICENSE Outdated
Comment thread docker/cloud-storage/scripts/test-graph-queries-and-sst.sh
vaijosh added 5 commits July 14, 2026 11:33
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.
@vaijosh
vaijosh requested a review from VGalaxies July 15, 2026 14:43

@VGalaxies VGalaxies left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 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
⚠️ Confirmed / limited 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

Comment thread install-dist/release-docs/LICENSE Outdated
Comment thread docker/cloud-storage/scripts/test-graph-queries-and-sst.sh Outdated
Comment thread docker/cloud-storage/scripts/test-graph-queries-and-sst.sh
@vaijosh

vaijosh commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

🚨 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
⚠️ Confirmed / limited 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

Hi @VGalaxies.
Thanks for reviewing the changes. I have addressed all the review comments. Can you please review it again?

@vaijosh

vaijosh commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

@VGalaxies
I am not sure why code coverage check is failing.
We have
The Diff has 94.79% coverage (agains target 42.32%) [codecov/patch](https://github.com/apache/hugegraph/pull/3081/checks?check_run_id=89150435397)Successful in 1s — 94.79% of diff hit (target 42.32%)

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

@vaijosh
vaijosh requested a review from VGalaxies July 23, 2026 11:57
vaijosh added a commit to vaijosh/hugegraph that referenced this pull request Oct 12, 2026
apache#3081
- Implemented checkstyle fixes and also other CI failures
vaijosh added a commit to vaijosh/hugegraph that referenced this pull request Oct 12, 2026
@vaijosh
vaijosh force-pushed the Hstore+CloudStorage branch from 1528cd0 to 1cf07b0 Compare October 13, 2026 04:40
vaijosh and others added 2 commits October 13, 2026 10:37
# 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>
vaijosh added a commit to vaijosh/hugegraph that referenced this pull request Oct 13, 2026
apache#3081
- Fix the docker image issues after the JDK 17 upgrade.
apache#3081
- Fix the docker image issues after the JDK 17 upgrade.
@vaijosh
vaijosh force-pushed the Hstore+CloudStorage branch from 8c00910 to bafbeae Compare October 13, 2026 07:33
vaijosh and others added 13 commits October 13, 2026 13:35
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.
-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

@bitflicker64 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@vaijosh
vaijosh force-pushed the Hstore+CloudStorage branch from 337ed3b to e8f494d Compare October 19, 2026 09:43
# 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
@vaijosh
vaijosh force-pushed the Hstore+CloudStorage branch from 14fc430 to ec9a692 Compare October 19, 2026 11:47
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 bitflicker64 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


@Override
public void onDBOpening(String dbName, String dbPath) {
if (!startupHydrationEnabled) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical: A pending-delete purge retry can delete the cloud data of a DB that was re-created at the same path.

The pending-delete guard runs only when startupHydrationEnabled is true. With startup-hydration-enabled=false this method returns before hasPendingDeleteMarker is checked. With hydration enabled, a failed inline tryCompletePendingDelete (cloud still down) logs and returns at L445-450, and the open still goes ahead. Either way the re-created DB starts uploading SSTs under the same prefix and markConfirmedIfEpoch confirms them. The retry scheduled by scheduleDeletePurgeRetry (or by scanMarkers at startup, L319-323) still sees the marker, so purgeRemotePrefix deletes those new objects. syncTracker is not cleared after the purge, so ensureSnapshotSstsUploaded (L1888) skips the deleted SSTs and CURRENT/MANIFEST are published pointing at objects that no longer exist. After a disk loss the DB cannot be restored. The javadoc at L925-929 assumes the inline cleanup always completes before the open, which is not true on these paths. The tombstone deletion after a successful purge is best-effort only (L651, L940), so a leftover tombstone makes the next restart's preHydrateDbFiles (L2068-2098) purge the live prefix too.

Do not open or upload into a prefix that still has a pending-delete marker, regardless of the hydration flag: block the open until cleanup succeeds, or give each DB incarnation its own key prefix. Call syncTracker.clearDb after every purge. Add a test that re-opens the DB while a delete purge retry is pending.

truncationTimes.put(dbName, System.currentTimeMillis());
try {
metadataSyncScheduler.schedule(() -> {
if (purgeRemotePrefix(provider, dbName, prefix)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical: The truncate purge retry purges the live prefix after the DB has resumed writing.

After graph.clear(), a failed purge is retried at 0.5, 1, 2, 4 and 8 s with no lock and no check that the DB is still empty. SST uploads are not suppressed during the truncate grace period (only deletes, metadata sync and compaction publish check it), so SSTs written after the truncate are uploaded and confirmed, and then this retry deletes them. The tracker is not cleared, so the next publish references missing objects, as in the pending-delete case. If all 5 retries fail, the marker stays, and on the next restart scanMarkers (L325-329) purges the whole prefix, which by then holds current live data. Since the tracker was seeded from the cloud listing, those files are not re-uploaded. Also, onDBOpening never checks pendingTruncateDir(), so a restart with the marker present and the local disk lost hydrates the cleared data back.

Finish the truncate purge before the DB accepts writes, or record the post-truncate state with the marker and abort the purge when the DB has moved past it. Check the truncate marker in onDBOpening. Clear the tracker after the purge.

// is loaded from a Spring Boot fat jar's nested BOOT-INF/lib, that classloader cannot see
// the listener classes, causing a NoClassDefFoundError storm on every callback. Skipping
// the registration when there is nothing to notify avoids the callback entirely.
if (RocksDBFactory.getInstance().hasRocksdbChangedListeners()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: The native RocksDB event listener is now attached for every store node, including nodes with cloud storage disabled.

hasRocksdbChangedListeners() is always true in a store node, because HgStoreEngine registers a listener via BusinessHandlerImpl.initRocksdb(..., getRocksdbListener()) and BusinessHandlerImpl registers another, both before any partition opens. At the merge base setListeners was never called, so these callbacks were never delivered. With this change every default deployment gets: an addBlankRaftTask() (a replicated Raft entry) on every compaction through HgStoreEngine.onCompacted; INFO logs on every compaction begin and SST delete; and the full JNI callback set, because RocksdbEventListener uses the no-arg AbstractEventListener constructor. It is also why the launcher had to move every user from -jar to -cp rocksdbjni:... JarLauncher and ship a second copy of rocksdbjni in lib/; any launcher that still uses -jar gets the NoClassDefFoundError storm the comment describes.

Attach the listener only when cloud storage is enabled (an explicit flag set by AppConfig, not "any listener registered"), construct it with only the callbacks it needs, and keep the old launch path when cloud storage is off. Add a test that a cloud-disabled store does not register the native listener.

String dbName = resolveDbName(dbNameOrPath, null);
// Skip metadata sync during truncation or grace period to allow purge to complete cleanly
if (!isActivelyTruncating(dbName) && !isInTruncationGracePeriod(dbName)) {
syncMetadataSnapshotInline(provider, dbName);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: Cloud I/O runs synchronously on RocksDB compaction threads.

onCompacted and onTableFileDeleted (L1407 ensureLiveSetUploaded, which can upload whole SSTs at L1602, and L1426 syncMetadataSnapshotInline) are invoked from RocksDB's native callbacks. A compaction with k inputs fires k delete callbacks plus one onCompacted, and each runs a full Checkpoint (which flushes the memtable, per the comment at L1770-1776), HEAD/PUT calls for unconfirmed SSTs, OPTIONS/MANIFEST/CURRENT uploads and a list-and-prune. Under write load the compaction threads are held for many S3 round trips while every checkpoint flush adds an L0 file, which leads to RocksDB write stalls.

Hand these events to a listener executor and publish once per compaction instead of once per deleted file.

*/
private void drainDirtyMetadata() {
for (String dbName : new ArrayList<>(dirtyMetadataDbs)) {
if (dirtyMetadataDbs.remove(dbName) && !publishDirtyMetadata(dbName)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important: A publish that keeps failing is retried every interval with no backoff, and each attempt adds a memtable flush and duplicate retry-queue entries.

When the publish fails (cloud outage, or missing write permission on metadata keys), the drain re-adds the DB and retries it every metadataSyncDebounceMs forever. Each attempt runs captureMetadataSnapshot, which flushes the memtable and creates a new L0 SST, and re-submits every unconfirmed SST to CloudUploadRetryQueue (L1913), which has no deduplication or bound. The queue grows roughly linearly with the outage length and L0 files accumulate. When the session is gone, captureMetadataSnapshot returns null and the DB is re-marked forever with no log. There is also a race with onDBDeleted (L643): the drain removes the DB, onDBDeleted removes it, and then the failed publish re-adds a deleted DB.

Back off per DB after a failure (or wait for the next mark), drop the DB when no session exists, and deduplicate retry-queue submissions by (db, file).

retries: 40
start_period: 120s

server:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: The committed static compose file starts the server without the HStore backend.

The server service sets no HG_SERVER_BACKEND: hstore, no PD peers and no graph-conf mount, unlike the compose file the test script generates. docker compose -f docker/cloud-storage/docker-compose.yml up therefore runs the server on the image's default backend, and data never reaches the stores or MinIO. Several README troubleshooting commands (for example around L163, L376-377 and L670) use this file and target the cloud-storage project, not the running cloud-storage-test project, so they return nothing. Both MinIO images are also unpinned :latest tags of a community fork.

Set the HStore backend and PD peers as docker/docker-compose-hstore.yml does, or remove the static file and point the README at the generated one. Pin the MinIO image versions.

watcher.deleteAttempts, e);
}
}
} else if (watcher.timestamp < (System.currentTimeMillis() - 1800 * 1000)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Destroy timing changes for users who never enable cloud storage.

Before this PR the branches were inverted, so a destroyed DB with live references was force-reset on the next pass and deleted about one to two minutes later. Now it waits 30 minutes, keeping the RocksDB LOCK and the disk space while partitions are rebalanced. The new order looks correct, and the PR also fixes watchers never being removed, but it is a cloud-unrelated, user-visible change.

Split it into its own PR, or call it out in the description and docs.

shutdownQuietly(dbSession);
return existing.clone();
}
dbSessionMap.put(dbName, dbSession);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: A creation that is still hydrating can outlive releaseAllGraphDB().

Phase 2 now runs outside the lock (before this PR it ran under it). If releaseAllGraphDB() runs while another thread is in Phase 2, Phase 3 inserts the session into the already-cleared map without re-checking closing. The session is never shut down, and onDBCreated fires, starting uploads, during shutdown.

Re-check closing in Phase 3 and shutdownQuietly the new session when it is set.

dbSession = new RocksDBSession(this.hugeConfig, dbPath, dbName, version);
dbSessionMap.put(dbName, dbSession);
// Spin-wait while another thread holds the pending-creation slot for this DB.
while (pendingCreations.contains(dbName)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Callers waiting for an in-flight creation spin on the global write lock with no timeout.

Every getSession call for a partition being hydrated reaches createGraphDB (BusinessHandlerImpl L1499-1504), and each waiter re-takes the global operateLock write lock every 10 ms. queryGraphDB for all other partitions contends with them, and if an S3 call hangs during hydration, request threads accumulate without a bound.

Wait on a per-name future or latch with a timeout instead.

return mapped;
}

Path name = Paths.get(normalised).getFileName();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Events delivered before onDBCreated resolve to the directory name instead of the logical DB name.

recordDb runs in onDBCreated, but RocksDB can fire callbacks earlier (for example a compaction scheduled at open). For a versioned directory such as 00001_5 the fallback returns 00001_5, so tracker confirmations land under the wrong name and the delete guard's slow path re-uploads those files. Per-DB meters are registered under db_name=00001_5 and unregisterDatabaseMetrics(logicalName) never removes them.

Call recordDb in onDBOpening, which already has both the name and the path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature size:XXL This PR changes 1000+ lines, ignoring generated files. store Store module

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

4 participants