Skip to content
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

[BEAM-13236] Properly close kinesis producer on teardown #15955

Merged
merged 5 commits into from Dec 7, 2021

Conversation

mosche
Copy link
Member

@mosche mosche commented Nov 12, 2021

Adds a missing producer.destroy() to KinesisWriterFn.teardown() .
Also, remove obsolete bugfix that is fixed in recent versions of KPL. See awslabs/amazon-kinesis-producer#34 (comment)


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

ValidatesRunner compliance status (on master branch)

Lang ULR Dataflow Flink Samza Spark Twister2
Go --- Build Status Build Status Build Status Build Status ---
Java Build Status Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Python --- Build Status
Build Status
Build Status
Build Status
Build Status
Build Status Build Status ---
XLang Build Status Build Status Build Status Build Status Build Status ---

Examples testing status on various runners

Lang ULR Dataflow Flink Samza Spark Twister2
Go --- --- --- --- --- --- ---
Java --- Build Status
Build Status
Build Status
--- --- --- --- ---
Python --- --- --- --- --- --- ---
XLang --- --- --- --- --- --- ---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go Java Python
Build Status Build Status Build Status
Build Status
Build Status

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website Whitespace Typescript
Non-portable Build Status
Build Status
Build Status
Build Status
Build Status
Build Status Build Status Build Status Build Status
Portable --- Build Status Build Status --- --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests

See CI.md for more information about GitHub Actions CI.

@mosche
Copy link
Member Author

mosche commented Nov 12, 2021

Run Java PreCommit

@mosche
Copy link
Member Author

mosche commented Nov 12, 2021

R: @aromanenko-dev
Just a small adhoc fix

@aromanenko-dev
Copy link
Contributor

Run Java PostCommit

Copy link
Contributor

@aromanenko-dev aromanenko-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
@mosche Could you create a new Jira for this fix and update commit message accordingly? Thanks

@mosche mosche force-pushed the adhoc-close-kinesis-producer branch from a22c3fd to 8abb52f Compare November 12, 2021 18:55
@mosche mosche changed the title [adhoc] Properly close kinesis producer on teardown [BEAM-13236] Properly close kinesis producer on teardown Nov 12, 2021
@mosche
Copy link
Member Author

mosche commented Nov 12, 2021

@aromanenko-dev done 👍

@mosche
Copy link
Member Author

mosche commented Nov 15, 2021

@echauchot Could you take over and merge?

@mosche
Copy link
Member Author

mosche commented Nov 17, 2021

Actually, this is more complicated than I thought. Closing the PR for now.

Per JVM a producer is shared among all KinesisWriterFns. Sharing producers absolutely makes sense, nevertheless this absolutely leaks. On teardown the shared(!) producer reference is unset, though it might still be required by another unfinished bundle running on the same JVM.

Also, if the static instance is reused, producer properties might be silently dropped without notice.

      private static transient IKinesisProducer producer;

@mosche mosche closed this Nov 17, 2021
@mosche mosche deleted the adhoc-close-kinesis-producer branch November 17, 2021 08:55
@echauchot
Copy link
Contributor

@mosche yes no problem, ping me on the new PR I'll do a quick review and merge if @aromanenko-dev is unavailable

@mosche mosche reopened this Nov 26, 2021
@mosche
Copy link
Member Author

mosche commented Nov 26, 2021

I've fixed initialisation and teardown of the Kinesis producer taking into account that one single producer (daemon) is shared among all writer instances of the same JVM.

@echauchot
Copy link
Contributor

thanks for this fix @mosche ! Reviewing ...

Copy link
Contributor

@echauchot echauchot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work @mosche ! And very good catch ! Only some minor changes but mostly LGTM

@mosche mosche force-pushed the adhoc-close-kinesis-producer branch from 18b88e7 to e93076f Compare November 30, 2021 05:28
@adude3141
Copy link
Contributor

LGTM.

@aromanenko-dev
Copy link
Contributor

Seems that this PR is ready for merge or something is left to do?

@mosche
Copy link
Member Author

mosche commented Dec 7, 2021

yes @aromanenko-dev, that's ready 👍

@aromanenko-dev aromanenko-dev merged commit ca50055 into apache:master Dec 7, 2021
@echauchot
Copy link
Contributor

thanks @aromanenko-dev for merging. I did not have time to give the final LGTM and merge

@adude3141
Copy link
Contributor

thx @aromanenko-dev for doing the merge here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants