syun64 commented on code in PR #265:
URL: https://github.com/apache/iceberg-python/pull/265#discussion_r1451651003


##########
pyiceberg/catalog/sql.py:
##########
@@ -268,16 +269,32 @@ def drop_table(self, identifier: Union[str, Identifier]) 
-> None:
         identifier_tuple = self.identifier_to_tuple_without_catalog(identifier)
         database_name, table_name = 
self.identifier_to_database_and_table(identifier_tuple, NoSuchTableError)
         with Session(self.engine) as session:
-            res = session.execute(
-                delete(IcebergTables).where(
-                    IcebergTables.catalog_name == self.name,
-                    IcebergTables.table_namespace == database_name,
-                    IcebergTables.table_name == table_name,
+            if self.engine.dialect.supports_sane_rowcount:
+                res = session.execute(
+                    delete(IcebergTables).where(
+                        IcebergTables.catalog_name == self.name,
+                        IcebergTables.table_namespace == database_name,
+                        IcebergTables.table_name == table_name,
+                    )
                 )
-            )
+                if res.rowcount < 1:
+                    raise NoSuchTableError(f"Table does not exist: 
{database_name}.{table_name}")
+            else:
+                try:
+                    tbl = (
+                        session.query(IcebergTables)
+                        .with_for_update(of=IcebergTables, nowait=True)

Review Comment:
   Great question @HonahX . The reason I wanted to avoid using skip_locked is 
because I think that would obfuscate the exception message we receive when 
NoResultFound is raised, which currently means that the table that we wanted to 
find doesn't exist in the DB.
   
   I felt that using nowait would be a good practice to avoid processes hanging 
and waiting for locks, which would behave a lot less like optimistic locking. 
With nowait, if we caught the right exception, we could raise a 
CommitFailedException with a message describing that a conflicting concurrent 
commit was made to the record, which is an expected type of exception with 
optimistic locking.
   
   Would it work to catch  sqlalchemy.exc.OperationalError that is raised when 
the lock isn't available?



-- 
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: issues-unsubscr...@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org

Reply via email to