stevenzwu edited a comment on pull request #2159: URL: https://github.com/apache/iceberg/pull/2159#issuecomment-772758415
@danielcweeks this is just an optimization for cases where async part upload failed. My understanding is that `completeMultiPartUpload` will fail during `close` time in this case of previous async part upload failure. Instead of waiting until close (which could be a few mins later), this change is trying to fail any writes fast and bubble up the async error to caller. Then jobs won't waste time on continuing writing and uploading data and maybe get restarted earlier. > I have concerns about introducing an atomic reference check into a critical section (like write(int)) I would imagine it probably won't matter much compared to IO write. Plus, you mentioned that it should not be used anyway. > for the multipart upload error, I feel like we we should not be catching and logging Are you referring to the new catch-exception in the `abortUpload` method? It is a cleanup action. We don't want to bubble error from `abortUpload`. Instead, we want to bubble up the original error (like part upload error). > We don't make guarantees around cleaning up multipart uploads That is true. This change doesn't change that aspect. still best effort. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
