bitflicker64 commented on PR #3081: URL: https://github.com/apache/hugegraph/pull/3081#issuecomment-6307560743
The earlier review comments below still apply at 084ae60; 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. (https://github.com/apache/hugegraph/pull/3081#discussion_r4325527810) - Critical: The truncate purge retry purges the live prefix after the DB has resumed writing. (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. (https://github.com/apache/hugegraph/pull/3081#discussion_r4325527826) - Important: Cloud I/O runs synchronously on RocksDB compaction threads. (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. (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. (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. (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. (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. (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. (https://github.com/apache/hugegraph/pull/3081#discussion_r4325527895) - Important: Providing credentials through environment variables crashes Store startup, even with cloud storage disabled. (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. (https://github.com/apache/hugegraph/pull/3081#discussion_r4325527921) - Important: The S3 provider tests never run in CI. (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. (https://github.com/apache/hugegraph/pull/3081#discussion_r4325527938) - Important: The tombstone sibling-key phase cannot fail, and a skip is recorded as PASS. (https://github.com/apache/hugegraph/pull/3081#discussion_r4325527949) - Minor: `close()` can lose the final metadata publish. (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. (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. (https://github.com/apache/hugegraph/pull/3081#discussion_r4325527985) - Minor: Setting `endpoint` without `region` fails Store startup with an unclear error. (https://github.com/apache/hugegraph/pull/3081#discussion_r4325527989) - Minor: `copy-dependencies` stages the whole HugeGraph dependency tree, not only the AWS SDK. (https://github.com/apache/hugegraph/pull/3081#discussion_r4325527999) - Minor: The NOTICE edits leave an empty section and remove existing separators. (https://github.com/apache/hugegraph/pull/3081#discussion_r4325528012) - Minor: The setup instructions describe a route that does not work with the shipped launcher. (https://github.com/apache/hugegraph/pull/3081#discussion_r4325528023) - Minor: RAT and editorconfig checks are disabled for every committed file under `docker/cloud-storage/`. (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. (https://github.com/apache/hugegraph/pull/3081#discussion_r4325528036) - Minor: The committed static compose file starts the server without the HStore backend. (https://github.com/apache/hugegraph/pull/3081#discussion_r4325528050) - Minor: Destroy timing changes for users who never enable cloud storage. (https://github.com/apache/hugegraph/pull/3081#discussion_r4325528057) - Minor: A creation that is still hydrating can outlive `releaseAllGraphDB()`. (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. (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. (https://github.com/apache/hugegraph/pull/3081#discussion_r4325528091) -- 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]
