Skip to content

MINOR: Apply try-with-resource to KafkaStreamsTest#10668

Merged
mjsax merged 1 commit into
apache:trunkfrom
vitojeng:apply-try-with-resources
Jun 3, 2021
Merged

MINOR: Apply try-with-resource to KafkaStreamsTest#10668
mjsax merged 1 commit into
apache:trunkfrom
vitojeng:apply-try-with-resources

Conversation

@vitojeng

@vitojeng vitojeng commented May 11, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

In the PR #9821, @mjsax 's comment

We should use a try-with-resources clause to make sure close() is called.

Seems, other tests in this class have a similar issue. Would be good to fix all test accordingly (if you want you can also to a separate PR for it).

Let me address this before the KAFKA-5876(KIP-216) part 5.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@vitojeng vitojeng force-pushed the apply-try-with-resources branch from 46babe0 to d91a8e4 Compare May 11, 2021 07:33
@vitojeng

vitojeng commented May 11, 2021

Copy link
Copy Markdown
Contributor Author

@mjsax , @ableegoldman
Please take a look. :)

chia7712
chia7712 reviewed May 11, 2021
View reviewed changes

@chia7712 chia7712 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@vitojeng thanks for this patch. one small comment below.

Comment thread streams/src/test/java/org/apache/kafka/streams/KafkaStreamsTest.java Outdated
@mjsax mjsax added the streams label May 11, 2021
mjsax
mjsax reviewed May 11, 2021
View reviewed changes
Comment thread streams/src/test/java/org/apache/kafka/streams/KafkaStreamsTest.java Outdated
mjsax
mjsax reviewed May 11, 2021
View reviewed changes
Comment thread streams/src/test/java/org/apache/kafka/streams/KafkaStreamsTest.java Outdated
@mjsax

mjsax commented Jun 2, 2021

Copy link
Copy Markdown
Member

@vitojeng -- Seems there is some conflicts. Can you rebase your PR so we can merge it?

@vitojeng vitojeng force-pushed the apply-try-with-resources branch from d91a8e4 to c53e84a Compare June 2, 2021 22:52
@vitojeng

vitojeng commented Jun 2, 2021

Copy link
Copy Markdown
Contributor Author

Thanks @mjsax !
Please take a look.🙂

@mjsax mjsax merged commit c2c08b4 into apache:trunk Jun 3, 2021
@mjsax

mjsax commented Jun 3, 2021

Copy link
Copy Markdown
Member

Thanks @vitojeng! Merged to trunk.

@vitojeng vitojeng deleted the apply-try-with-resources branch June 3, 2021 04:35
@lyxell lyxell mentioned this pull request Jun 17, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chia7712 chia7712 chia7712 left review comments

@mjsax mjsax mjsax left review comments

Assignees

No one assigned

Labels

streams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vitojeng @mjsax @chia7712