AWS,Azure: Fix S3InputStream and ADLSInputStream connection leaks#13905
Conversation
A follow-up to apache#13899 with more fixes of the same issue.
68f0f72 to
2ef33fe
Compare
| import software.amazon.awssdk.services.s3.S3Client; | ||
| import software.amazon.awssdk.services.s3.model.GetObjectRequest; | ||
|
|
||
| @ExtendWith(MockitoExtension.class) |
There was a problem hiding this comment.
I think having the separate unit test for the input stream in addition to the existing integration test is probably a good thing, since we can verify the stream reading logic independent of any behavior in the underlying client, and it'd be nice to run some tests here that don't require any environment setup.
At the moment it looks like we just assert the stream is closed but we may want to see if it's possible to build a shared abstraction between the unit and integration test which allows being able to plug in whichever client is desired; in the unit test case, a mocked client is plugged in and in the integration test case, a real client is plugged in. Then if there are any other clients out there someone could easily extend the test and run that.
|
Overall looks good to me thank you @AnatolyPopov and @nandorKollar for reviewing. Just had a future looking comment on our testing but nothing necessary in this PR imo. |
A follow-up to #13899 with more fixes of the same issue.