FIX: Exception Handling in AWS Glue renameTable Method #11165
Conversation
This PR addresses an issue in the AWS Glue renameTable method where an incorrect exception is thrown when a table with the new name already exists. According to the Iceberg Catalog interface, the method should throw org.apache.iceberg.exceptions.AlreadyExistsException. However, it currently throws software.amazon.awssdk.services.glue.model.AlreadyExistsException, leading to inconsistent behavior. Problem: The renameTable method throws an AWS Glue-specific AlreadyExistsException instead of the expected Iceberg-specific AlreadyExistsException. This violates the contract defined by the Iceberg Catalog interface and can cause confusion for applications relying on the Iceberg exception handling. Solution: Updated the glue.createTable() call: The AlreadyExistsException from AWS Glue is now caught and rethrown as Iceberg’s org.apache.iceberg.exceptions.AlreadyExistsException. Ensures that the Iceberg exception is consistently used in line with the catalog interface specification. Changes: Wrapped the glue.createTable() call in a try-catch block. Caught software.amazon.awssdk.services.glue.model.AlreadyExistsException and rethrew it as org.apache.iceberg.exceptions.AlreadyExistsException with an appropriate error message. Retained the existing rollback logic for handling failures when dropping the old table. Impact: Ensures consistent exception handling in the renameTable method. Aligns the behavior with the Iceberg API contract. No impact on backward compatibility, as this only corrects exception handling. This PR ensures that the renameTable operation behaves as expected according to the Iceberg documentation and resolves the mismatch in exception handling.
|
Can somebody please review my PR ? |
There was a problem hiding this comment.
Thanks for the fix @jishangarg, overall the fix looks good to me but I think we need a test for this. Could we add one to TestGlueCatalog please?
| LOG.info("created rename destination table {}", to); | ||
| } catch (software.amazon.awssdk.services.glue.model.AlreadyExistsException e) { | ||
| // Catch AWS Glue exception and rethrow it as Iceberg's AlreadyExistsException | ||
| throw new org.apache.iceberg.exceptions.AlreadyExistsException("Table already exists: " + toTableName, e); |
There was a problem hiding this comment.
Can we use the fully qualified software.amazon.awssdk.services.glue.model.AlreadyExistsException for glue and then just use the normal class name for the Iceberg AlreadyExistsException. That aligns with what's done in the other parts of the code and is a bit more readable imo.
|
Also double check spotless, seems like those checks are failing. You can run |
|
The particular case referred here makes sense but was thinking if this is something we need to handle broadly ? @amogh-jahagirdar was thinking if we can re-use / generalize this code or classification [1] and wire it to all all glue-clients request / response interaction, your thoughts ? |
|
Thanks @singhpk234! At the moment the catalog API for renameTable does not specify what the expected exceptions are for the 403 or throttling, so at the moment the Glue exception being surfaced for those cases doesn't really violate the interface but I agree in principle that we may as well make those Iceberg exceptions in the catalog implementation. The part to consider here is downstream clients who may already be relying on the previous exception behavior and specifically handling Glue exceptions. The already exists case is a bit more clear cut imo since the API defines the expected behavior for implementations and right now the impl is violating that. Overall though, I wouldn't be opposed to upleveling the glue to iceberg exception conversion logic in |
Related to issue: #11155
This PR addresses an issue in the AWS Glue renameTable method where an incorrect exception is thrown when a table with the new name already exists. According to the Iceberg Catalog interface, the method should throw org.apache.iceberg.exceptions.AlreadyExistsException. However, it currently throws software.amazon.awssdk.services.glue.model.AlreadyExistsException, leading to inconsistent behavior.
Problem:
The renameTable method throws an AWS Glue-specific AlreadyExistsException instead of the expected Iceberg-specific AlreadyExistsException. This violates the contract defined by the Iceberg Catalog interface and can cause confusion for applications relying on the Iceberg exception handling.
Solution:
Updated the glue.createTable() call: The AlreadyExistsException from AWS Glue is now caught and rethrown as Iceberg’s org.apache.iceberg.exceptions.AlreadyExistsException. Ensures that the Iceberg exception is consistently used in line with the catalog interface specification.
Changes:
Wrapped the glue.createTable() call in a try-catch block. Caught software.amazon.awssdk.services.glue.model.AlreadyExistsException and rethrew it as org.apache.iceberg.exceptions.AlreadyExistsException with an appropriate error message. Retained the existing rollback logic for handling failures when dropping the old table.
Impact:
Ensures consistent exception handling in the renameTable method. Aligns the behavior with the Iceberg API contract. No impact on backward compatibility, as this only corrects exception handling. This PR ensures that the renameTable operation behaves as expected according to the Iceberg documentation and resolves the mismatch in exception handling.