hevinhsu commented on PR #9294: URL: https://github.com/apache/ozone/pull/9294#issuecomment-3696189980
Hi @ivandika3, Thank you for taking the time to review this fairly large change set and for the detailed suggestions — I really appreciate it. > **Regarding updating the BucketEndpoint issue, could you try changing the `catch (IOException)` to `catch (OMException)` instead…** I tried changing `catch (IOException)` to `catch (OMException)`, but this did not work for me. The root cause of the failing tests comes from the bucket ownership check, where the exception is thrown from [`S3Owner`](https://github.com/apache/ozone/blob/master/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/S3Owner.java#L117). To address this, I added an explicit `catch (O3SException ex)` block in [`BucketEndpoint`]( https://github.com/hevinhsu/ozone/blob/HDDS-13668_test-IOException/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java#L508-L511). With this change, the CI passed. This approach avoids using `instanceof` checks while still handling the error correctly. > **Another possible alternative is simply make OS3Exception to be unchecked (RuntimeException).** I also tried making `O3SException` an unchecked exception, and so far everything looks good — all tests passed without requiring additional catch logic. ([CI](https://github.com/hevinhsu/ozone/actions/runs/20565921336)) From an implementation-simplicity perspective, this option seems quite attractive, and I think it’s worth considering. > **Wrapper solution / `HddsClientUtils#containsException`** I also tried the wrapper-based approach by introducing [S3IOExceptionWrapper](https://github.com/hevinhsu/ozone/blob/HDDS-13668_wrapper-solution/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3IOExceptionWrapper.java) to carry the original `O3SException`. I did not end up using `HddsClientUtils#containsException`, mainly because this solution felt a bit complex for the use case here. By directly adding a [cache(S3IOExceptionWrapper wrapper)](https://github.com/hevinhsu/ozone/blob/HDDS-13668_wrapper-solution/hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java#L399-L401) block, I was able to achieve the intended behavior in a more straightforward way.([CI](https://github.com/hevinhsu/ozone/actions/runs/20569494114)) If there are any edge cases or design considerations that I might have overlooked with this simplified approach, please let me know — I’d be happy to revisit it. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
