jackye1995 commented on a change in pull request #2159:
URL: https://github.com/apache/iceberg/pull/2159#discussion_r569092361
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3OutputStream.java
##########
@@ -280,11 +300,12 @@ private void completeMultiPartUpload() {
.run(s3::completeMultipartUpload);
}
- private void abortUpload() {
+ private synchronized void abortUpload() {
Review comment:
Thanks for noticing the multipartUploadId is not set back to null.
But I don't think we need to make it synchronized. If two threads both try
to abort, one will just fail, and the finally block also does not throw
exception. Do you see any issue when it is not synchronized?
##########
File path: aws/src/test/java/org/apache/iceberg/aws/s3/S3OutputStreamTest.java
##########
@@ -133,6 +135,64 @@ public void testAbortMultipart() {
}
}
+ public void testFailWriteIntAfterAsyncError() throws Exception {
+ doThrow(new
RuntimeException()).when(s3mock).uploadPart((UploadPartRequest) any(),
(RequestBody) any());
Review comment:
nit: the code from L139-L146 is heavily repeated, we can move it to a
helper private method
----------------------------------------------------------------
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]
