Skip to content

Conversation

@feeblefakie
Copy link
Contributor

This is an automated request for a manual backport of the following:

  1. Resolve any conflicts that occur during the cherry-picking process.
git fetch origin &&
git checkout 3-pull-3092 &&
git cherry-pick --no-rerere-autoupdate -m1 024172e8a650f26bfc3019642932e0d3424c303b
  1. Push the changes.
  2. Merge this PR after all checks have passed.

Thank you!

@KodaiD
Copy link
Contributor

KodaiD commented Oct 30, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request backports a fix to prevent dropping namespaces that contain non-ScalarDB managed tables. The changes add the necessary checks for CosmosDB and various JDBC backends.

However, I've found a critical issue in the implementation for both CosmosAdmin and JdbcAdmin. The logic in dropNamespace is incorrect, as it doesn't first check for ScalarDB-managed tables before checking for any other tables. This can lead to incorrect error messages or behavior. I've provided suggestions to correct this logic.

Additionally, it appears that the corresponding changes for CassandraAdmin are missing from this backport. Please ensure that the Cassandra implementation is also updated to include this fix.

The rest of the changes, including updates to tests and test utilities, look good and correctly support the new functionality.

@KodaiD
Copy link
Contributor

KodaiD commented Oct 30, 2025

Code Review

This pull request backports a fix to prevent dropping namespaces that contain non-ScalarDB managed tables. The changes add the necessary checks for CosmosDB and various JDBC backends.

However, I've found a critical issue in the implementation for both CosmosAdmin and JdbcAdmin. The logic in dropNamespace is incorrect, as it doesn't first check for ScalarDB-managed tables before checking for any other tables. This can lead to incorrect error messages or behavior. I've provided suggestions to correct this logic.

Additionally, it appears that the corresponding changes for CassandraAdmin are missing from this backport. Please ensure that the Cassandra implementation is also updated to include this fix.

The rest of the changes, including updates to tests and test utilities, look good and correctly support the new functionality.

Incorrect. CommonDistributedStorageAdmin checks ScalarDB tables. Also, CassandraAdmin does not need the fix because CommonDistributedStorageAdmin can detect both ScalarDB tables and non-ScalarDB tables with Cassandra.

@KodaiD KodaiD marked this pull request as ready for review October 30, 2025 10:49
@KodaiD KodaiD merged commit 34c5abf into 3 Oct 30, 2025
126 of 128 checks passed
@KodaiD KodaiD deleted the 3-pull-3092 branch October 30, 2025 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants