vaijosh commented on PR #3081: URL: https://github.com/apache/hugegraph/pull/3081#issuecomment-6345274228
> The earlier review comments below still apply at [084ae60](https://github.com/apache/hugegraph/commit/084ae6062eef730da5a4cfddcfd85f8ce2854a31); this head does not fix them. > > * Critical: A pending-delete purge retry can delete the cloud data of a DB that was re-created at the same path. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527810)) > * Critical: The truncate purge retry purges the live prefix after the DB has resumed writing. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527818)) > * Important: The native RocksDB event listener is now attached for every store node, including nodes with cloud storage disabled. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527826)) > * Important: Cloud I/O runs synchronously on RocksDB compaction threads. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527839)) > * 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. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527849)) > * Important: Dirty marks are dropped while a DB is truncating or in its grace period, so the cloud CURRENT/MANIFEST can stay stale indefinitely. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527859)) > * Important: Startup hydration lists only the base DB directory prefix, so a partition that moved to a versioned directory is not restored. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527872)) > * Important: The destroy-then-recreate guard compares only the DB name, so a destroyed DB's directory is never deleted and `onDBDeleted` never fires. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527877)) > * Important: The metadata checkpoint is categorised after the lock is released, and its callers hold no session reference, so a concurrent release can NPE or use a closed native handle. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527886)) > * Important: `<dbPath>_cloudmeta_<nanos>` checkpoint directories left by a crash are never removed and are read as partition data. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527895)) > * Important: Providing credentials through environment variables crashes Store startup, even with cloud storage disabled. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527910)) > * Important: The SDK's default request checksums are left on, which breaks PutObject against many S3-compatible endpoints the PR targets. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527921)) > * Important: The S3 provider tests never run in CI. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527928)) > * Important: The recovery check compares vertex counts that are capped at 100, so it passes when up to a third of the data is lost. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527938)) > * Important: The tombstone sibling-key phase cannot fail, and a skip is recorded as PASS. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527949)) > * Minor: `close()` can lose the final metadata publish. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527962)) > * Minor: The `.hyd-tmp-` sweep runs only when startup hydration is enabled and the remote listing is non-empty. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527976)) > * Minor: The read-miss hook on the null-result path costs work but cannot change the result. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527985)) > * Minor: Setting `endpoint` without `region` fails Store startup with an unclear error. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527989)) > * Minor: `copy-dependencies` stages the whole HugeGraph dependency tree, not only the AWS SDK. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325527999)) > * Minor: The NOTICE edits leave an empty section and remove existing separators. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325528012)) > * Minor: The setup instructions describe a route that does not work with the shipped launcher. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325528023)) > * Minor: RAT and editorconfig checks are disabled for every committed file under `docker/cloud-storage/`. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325528031)) > * Minor: The documented metric names and labels do not match the code, so the documented alerts would never fire. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325528036)) > * Minor: The committed static compose file starts the server without the HStore backend. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325528050)) > * Minor: Destroy timing changes for users who never enable cloud storage. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325528057)) > * Minor: A creation that is still hydrating can outlive `releaseAllGraphDB()`. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325528063)) > * Minor: Callers waiting for an in-flight creation spin on the global write lock with no timeout. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325528080)) > * Minor: Events delivered before `onDBCreated` resolve to the directory name instead of the logical DB name. ([feat(store): implement pluggable cloud storage for HStore #3081 (comment)](https://github.com/apache/hugegraph/pull/3081#discussion_r4325528091)) Thanks @bitflicker64 for the thorough review. All items in your list are addressed. Can you please take a look? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
