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

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

krvikash commented on code in PR #6500:
URL: https://github.com/apache/iceberg/pull/6500#discussion_r1058723448


##########
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:
   Thanks, @amogh-jahagirdar for the review.
   
   I am not changing the meaning of the code as compared to earlier. It will work the same way as earlier. Since `commitStatus` is already set to `CommitStatus.FAILURE` at the beginning of `doCommit` method then we don't need to set it again when `AwsServiceException` `statusCode` >= 500 and < 600.
   
   
   > A retry commit is performed which fails with a status code >= 500 and < 600 then commit status then falls through to the checkCommitStatus again 
   
   No that's not true, `checkCommitStatus` will be only called for non-AwsServiceException. Any `AwsServiceException` which does not fall under >= 500 and < 600 will have the exception `persistFailure` and others will have to go through `switch` statement and throw the exception according to `commitStatus` value (in this case it will always be `CommitStatus.FAILURE`).
   
   For simplicity and readability, the `statusCode` check can be changed to,
   
   ```java
   if (!(statusCode >= 500 && statusCode < 600)) {
        throw persistFailure;
   }
   ```
   
   My whole point was to remove redundant assigning to `commitStatus` since it was already set at the beginning of this method call.
   
   



-- 
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