[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #6500: Aws: Cosmetic change and simplify statusCode check in GlueTableOperations

[GitBox <gi...@apache.org>]

amogh-jahagirdar commented on code in PR #6500:
URL: https://github.com/apache/iceberg/pull/6500#discussion_r1058674867


##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -184,9 +185,7 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
 
       if (persistFailure instanceof AwsServiceException) {
         int statusCode = ((AwsServiceException) persistFailure).statusCode();
-        if (statusCode >= 500 && statusCode < 600) {
-          commitStatus = CommitStatus.FAILURE;
-        } else {
+        if (statusCode < 500 && statusCode >= 600) {

Review Comment:
   Not entirely sure about this change considering the presence of commit retries and checkCommitStatus being a heavy operation.
   
    Is the following a possible sequence of events? : 
    
    1.) The operation fails to commit due to some non AwsServiceException, so commitStatus is set to something else.
   2.) A retry commit is performed which fails with a status code >= 500 and < 600 then commit status then falls through to the checkCommitStatus again which is heavier since it has to get the latest metadata location via a Glue call. 
   
   In step 2 we know it's a failure case immediately instead of having to do a checkCommitStatus. 
   
   
   
   
    
   
   



-- 
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-unsubscribe@iceberg.apache.org

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


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