You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/07/20 19:06:52 UTC
[GitHub] [iceberg] abmo-x opened a new pull request, #5317: unit test should always verify mock invocation
abmo-x opened a new pull request, #5317:
URL: https://github.com/apache/iceberg/pull/5317
current implementation only verifies mock invocation on an exception in catch block. This is flaky as the test won't verify if there are no failures, but we want to make sure the calls are made in the test.
--
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
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #5317: unit test should always verify mock invocation
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #5317:
URL: https://github.com/apache/iceberg/pull/5317#discussion_r926154013
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java:
##########
@@ -168,7 +179,8 @@ private void writeTest() {
writeAndVerify(s3mock, randomURI(), data, arrayWrite);
ArgumentCaptor<PutObjectRequest> putObjectRequestArgumentCaptor =
ArgumentCaptor.forClass(PutObjectRequest.class);
- verify(s3mock, times(1)).putObject(putObjectRequestArgumentCaptor.capture(),
+ verify(s3mock, times(1)).putObject(
Review Comment:
Please revert format changes
--
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
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #5317: unit test should always verify mock invocation
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #5317:
URL: https://github.com/apache/iceberg/pull/5317#discussion_r926000605
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java:
##########
@@ -119,22 +119,26 @@ public void testWrite() {
@Test
public void testAbortAfterFailedPartUpload() {
- doThrow(new RuntimeException()).when(s3mock).uploadPart((UploadPartRequest) any(), (RequestBody) any());
+ doThrow(new RuntimeException("mock uploadPart failure")).when(s3mock).uploadPart((UploadPartRequest) any(), (RequestBody) any());
try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) {
stream.write(randomData(10 * 1024 * 1024));
} catch (Exception e) {
Review Comment:
https://github.com/apache/iceberg/pull/5221 <- For more context
```
Assertions.assertThatThrownBy(() -> timestampStr.to(Types.TimestampType.withoutZone()))
.isInstanceOf(DateTimeException.class)
.hasMessageContaining("could not be parsed");
```
--
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
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #5317: unit test should always verify mock invocation
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #5317:
URL: https://github.com/apache/iceberg/pull/5317#discussion_r926153528
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java:
##########
@@ -72,7 +73,6 @@
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
-
Review Comment:
un-needed whitespace change
--
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
[GitHub] [iceberg] singhpk234 commented on pull request #5317: unit test should always verify mock invocation
Posted by GitBox <gi...@apache.org>.
singhpk234 commented on PR #5317:
URL: https://github.com/apache/iceberg/pull/5317#issuecomment-1198885093
+1, I also observed this, have a possible RC, presently in case of any failure of completable future in `uploadParts` we call abortUpload() which further deletes all the staging files https://github.com/apache/iceberg/blob/3d00780f5f508ac7a0f475fd0e1ddfd736000c5b/aws/src/main/java/org/apache/iceberg/aws/s3/S3OutputStream.java#L377-L389
Now when another completable future starts to read file for creating a request https://github.com/apache/iceberg/blob/3d00780f5f508ac7a0f475fd0e1ddfd736000c5b/aws/src/main/java/org/apache/iceberg/aws/s3/S3OutputStream.java#L323-L328 it fails with a FileNotFoundException (as staging files have been deleted from earlier future) Now when we do join of all the completable future we can get a failure due to `FileNotFound`.
here is a gist for repro consistently & complete stack trace : https://gist.github.com/singhpk234/4257ea980017db5704857c3c7cc2fd0b
I think this PR of mine can fix this flakyness as well : https://github.com/apache/iceberg/pull/5366
--
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
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #5317: unit test should always verify mock invocation
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #5317:
URL: https://github.com/apache/iceberg/pull/5317#discussion_r925999644
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java:
##########
@@ -119,22 +119,26 @@ public void testWrite() {
@Test
public void testAbortAfterFailedPartUpload() {
- doThrow(new RuntimeException()).when(s3mock).uploadPart((UploadPartRequest) any(), (RequestBody) any());
+ doThrow(new RuntimeException("mock uploadPart failure")).when(s3mock).uploadPart((UploadPartRequest) any(), (RequestBody) any());
try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) {
stream.write(randomData(10 * 1024 * 1024));
} catch (Exception e) {
Review Comment:
I think for something like this we should use the
```
Assertions.assertThrows(RuntimeException).when( S3OutputStream ....)
verify(...)
```
--
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
[GitHub] [iceberg] abmo-x commented on pull request #5317: unit test should always verify mock invocation
Posted by GitBox <gi...@apache.org>.
abmo-x commented on PR #5317:
URL: https://github.com/apache/iceberg/pull/5317#issuecomment-1197454587
It's possible this exception was occurring before and the test ignored it as the test was catching all exceptions before this change. Now we are explicitly checking for the exception to be of certain type with certain msg.
Not sure if there is some race condition going on which causes file to be [deleted on exit](https://github.com/apache/iceberg/blob/dbb8a404f6632a55acb36e949f0e7b84b643cede/aws/src/main/java/org/apache/iceberg/aws/s3/S3OutputStream.java#L221) or some parallel test which is cleaning up the tmp dir
--
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
[GitHub] [iceberg] RussellSpitzer commented on pull request #5317: unit test should always verify mock invocation
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on PR #5317:
URL: https://github.com/apache/iceberg/pull/5317#issuecomment-1197431788
Haven't taken a look yet but @abmo-x we do run the tests as you will see if you click the "details" on the merge note above. All tests passed at that time.
--
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
[GitHub] [iceberg] aokolnychyi commented on pull request #5317: unit test should always verify mock invocation
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on PR #5317:
URL: https://github.com/apache/iceberg/pull/5317#issuecomment-1196164678
@RussellSpitzer @abmo-x, we have started seeing test failures in affected tests. Could you help investigate?
```
org.apache.iceberg.aws.s3.TestS3OutputStream > testAbortAfterFailedPartUpload FAILED
[213](https://github.com/apache/iceberg/runs/7530666701?check_suite_focus=true#step:6:214)
java.lang.AssertionError:
[214](https://github.com/apache/iceberg/runs/7530666701?check_suite_focus=true#step:6:215)
Expecting throwable message:
[215](https://github.com/apache/iceberg/runs/7530666701?check_suite_focus=true#step:6:216)
"java.io.UncheckedIOException: java.nio.file.NoSuchFileException: /tmp/s3fileio-test-3731532446800745900/s3fileio-2448969152692915450.tmp"
[216](https://github.com/apache/iceberg/runs/7530666701?check_suite_focus=true#step:6:217)
to contain:
[217](https://github.com/apache/iceberg/runs/7530666701?check_suite_focus=true#step:6:218)
"mock uploadPart failure"
[218](https://github.com/apache/iceberg/runs/7530666701?check_suite_focus=true#step:6:219)
but did not.
[219](https://github.com/apache/iceberg/runs/7530666701?check_suite_focus=true#step:6:220)
```
--
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
[GitHub] [iceberg] aokolnychyi commented on pull request #5317: unit test should always verify mock invocation
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on PR #5317:
URL: https://github.com/apache/iceberg/pull/5317#issuecomment-1196208289
Looks like it fails sporadically. I'll look into it tomorrow unless @RussellSpitzer gets to it first.
--
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
[GitHub] [iceberg] abmo-x commented on pull request #5317: unit test should always verify mock invocation
Posted by GitBox <gi...@apache.org>.
abmo-x commented on PR #5317:
URL: https://github.com/apache/iceberg/pull/5317#issuecomment-1196207442
> testAbortAfterFailedPartUpload
@aokolnychyi looks like the expected tmp staging file that gets created disappeared, this failure occurs before the mock gets invoked.
https://github.com/apache/iceberg/runs/7530666701?check_suite_focus=true#step:6:239
```
Caused by: java.nio.file.NoSuchFileException: /tmp/s3fileio-test-3731532446800745900/s3fileio-2448969152692915450.tmp
```
Wondering what could be different in the test environment, the test works fine when ran locally both from cli and IDE
--
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
[GitHub] [iceberg] amogh-jahagirdar commented on pull request #5317: unit test should always verify mock invocation
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on PR #5317:
URL: https://github.com/apache/iceberg/pull/5317#issuecomment-1198813688
Yeah it's failing for me too. https://github.com/apache/iceberg/runs/7571542364?check_suite_focus=true
I'm also taking a look into 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: 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
[GitHub] [iceberg] RussellSpitzer merged pull request #5317: unit test should always verify mock invocation
Posted by GitBox <gi...@apache.org>.
RussellSpitzer merged PR #5317:
URL: https://github.com/apache/iceberg/pull/5317
--
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
[GitHub] [iceberg] abmo-x commented on pull request #5317: unit test should always verify mock invocation
Posted by GitBox <gi...@apache.org>.
abmo-x commented on PR #5317:
URL: https://github.com/apache/iceberg/pull/5317#issuecomment-1190712956
Updated with recommendations, Thanks @RussellSpitzer
--
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
[GitHub] [iceberg] abmo-x commented on pull request #5317: unit test should always verify mock invocation
Posted by GitBox <gi...@apache.org>.
abmo-x commented on PR #5317:
URL: https://github.com/apache/iceberg/pull/5317#issuecomment-1190907221
> I think there are a few formatting issues remaining but other than that this is good to go. We need to revert all the changes which aren't directly related to the patch. I think the byte [] to byte[] is correct but we should separate that out into another patch. Since we are about to do a big programmatic style application soon it is probably best to just leave it as is for now.
Reverted formatting which was not part of the PR. Thanks!
--
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
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #5317: unit test should always verify mock invocation
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #5317:
URL: https://github.com/apache/iceberg/pull/5317#discussion_r926000605
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java:
##########
@@ -119,22 +119,26 @@ public void testWrite() {
@Test
public void testAbortAfterFailedPartUpload() {
- doThrow(new RuntimeException()).when(s3mock).uploadPart((UploadPartRequest) any(), (RequestBody) any());
+ doThrow(new RuntimeException("mock uploadPart failure")).when(s3mock).uploadPart((UploadPartRequest) any(), (RequestBody) any());
try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) {
stream.write(randomData(10 * 1024 * 1024));
} catch (Exception e) {
Review Comment:
https://github.com/apache/iceberg/pull/5221 <- For more context
--
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
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #5317: unit test should always verify mock invocation
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #5317:
URL: https://github.com/apache/iceberg/pull/5317#discussion_r926007484
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestS3OutputStream.java:
##########
@@ -119,24 +120,28 @@ public void testWrite() {
@Test
public void testAbortAfterFailedPartUpload() {
- doThrow(new RuntimeException()).when(s3mock).uploadPart((UploadPartRequest) any(), (RequestBody) any());
+ doThrow(new RuntimeException("mock uploadPart failure"))
+ .when(s3mock).uploadPart((UploadPartRequest) any(), (RequestBody) any());
- try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) {
- stream.write(randomData(10 * 1024 * 1024));
- } catch (Exception e) {
- verify(s3mock, atLeastOnce()).abortMultipartUpload((AbortMultipartUploadRequest) any());
- }
+ Assertions.assertThrows(RuntimeException.class, ()->{
+ try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) {
+ stream.write(randomData(10 * 1024 * 1024));
+ }
+ });
+ verify(s3mock, atLeastOnce()).abortMultipartUpload((AbortMultipartUploadRequest) any());
}
@Test
public void testAbortMultipart() {
- doThrow(new RuntimeException()).when(s3mock).completeMultipartUpload((CompleteMultipartUploadRequest) any());
+ doThrow(new RuntimeException("mock completeMultipartUpload failure"))
+ .when(s3mock).completeMultipartUpload((CompleteMultipartUploadRequest) any());
- try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) {
- stream.write(randomData(10 * 1024 * 1024));
- } catch (Exception e) {
- verify(s3mock).abortMultipartUpload((AbortMultipartUploadRequest) any());
- }
+ Assertions.assertThrows(RuntimeException.class, ()->{
+ try (S3OutputStream stream = new S3OutputStream(s3mock, randomURI(), properties, nullMetrics())) {
+ stream.write(randomData(10 * 1024 * 1024));
+ }
+ });
Review Comment:
We should probably add checking the message since that was added as well, Also please be sure to get that checkstyle running ```gradle build -x test -x integrationTest``` should run all the static checks.
--
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