-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[FLINK-26006][connector-aws-kinesis-firehose] Avoid leaking AWS SDK r… #18661
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 0f18fcb (Tue Feb 08 11:17:21 UTC 2022) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. DetailsThe Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
| } | ||
|
|
||
| public static AwsCredentialsProvider getDefaultCredentials() { | ||
| public static AwsCredentialsProvider createDefaultCredentials() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review would've been easier if the renamings wouldn't be there :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to make explicit that EVERY SINGLE CALL to these methods, create a resource that needs to be closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a fair point!
zentol
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me; but I'm very concerned that something as basic as closing clients isn't being done.
We should probably verify the whole module in that case...
|
@zentol @dannycranmer The very same problem is with other AWS code that has been added recently. |
| public static final String KINESALITE = "instructure/kinesalite:latest"; | ||
|
|
||
| public static final String LOCALSTACK = "localstack/localstack:latest"; | ||
| public static final String LOCALSTACK = "localstack/localstack:0.13.3"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dannycranmer This can backfire at any time (and it already did, but the docker cache hides it on CI). Same should apply for the kinesis above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, thanks for the spot. We will update the kinesalite image https://issues.apache.org/jira/browse/FLINK-26008
|
@zentol apologies this was missed on the first PRs, however we had detected and raised a Jira to fix it https://issues.apache.org/jira/browse/FLINK-25977 |
https://issues.apache.org/jira/browse/FLINK-26006