Skip to content

KAFKA-12380 Executor in Connect's Worker is not shut down when the worker is#11955

Merged
showuon merged 3 commits intoapache:trunkfrom
karuturi:bigfix/KAFKA-12380-2
Apr 29, 2022
Merged

KAFKA-12380 Executor in Connect's Worker is not shut down when the worker is#11955
showuon merged 3 commits intoapache:trunkfrom
karuturi:bigfix/KAFKA-12380-2

Conversation

@karuturi
Copy link
Copy Markdown
Member

added shutdown for executors and tests for the same

Committer Checklist (excluded from commit message)

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

@karuturi
Copy link
Copy Markdown
Member Author

I could not find the exact failed testcases. I can see assertion errors in StreamsUncaughtExceptionHandlerIntegrationTest. If I run it locally, it's taking forever.

Can someone help me find the failed tests?
The change I added is very isolated. Should not cause any failures ideally.

@karuturi
Copy link
Copy Markdown
Member Author

I downloaded the console log and a grep showed below tests

cat consoleText.txt| grep FAILED
[2022-03-28T14:02:18.545Z] SocketServerTest > testIdleConnection() FAILED
[2022-03-28T14:44:11.437Z] TransactionsBounceTest > testWithGroupId() FAILED
[2022-03-28T14:59:52.526Z] DynamicConnectionQuotaTest > testDynamicListenerConnectionCreationRateQuota() FAILED
[2022-03-28T15:04:10.646Z] KRaftClusterTest > testLegacyAlterConfigs() FAILED

all of them are successful if ran individually

@showuon
Copy link
Copy Markdown
Member

showuon commented Apr 1, 2022

@karuturi , thanks for the PR. But I found there was a long pending PR that is addressing the same issue (#10337). Could we wait for a few days to see if the author wants to continue the old PR or not, before we start review this PR? (I've left a comment in that PR).

Thanks.

@karuturi
Copy link
Copy Markdown
Member Author

karuturi commented Apr 4, 2022

@showuon Sure, can wait.
Hope we will merge one of them soon :)

@karuturi
Copy link
Copy Markdown
Member Author

karuturi commented Apr 7, 2022

Hi @showuon
As there is no response on the other PR, can we take this forward?

…rker is

added shutdown for executors and tests for the same
@karuturi karuturi force-pushed the bigfix/KAFKA-12380-2 branch from 57b6a39 to 997cf48 Compare April 7, 2022 04:28
@karuturi
Copy link
Copy Markdown
Member Author

@showuon Can we merge this please?

Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

LGTM! Left one comment. And thanks for the tests.

…rker is

preserving the current thread state during interrupt
Copy link
Copy Markdown
Member

@showuon showuon 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 the update. Looks better. One minor comment left.

Comment thread connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java Outdated
Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the patch. Let's wait for the jenkins build results.

@karuturi
Copy link
Copy Markdown
Member Author

Jenkins has failures unrelated to this PR

@showuon
Copy link
Copy Markdown
Member

showuon commented Apr 29, 2022

Failed tests are unrelated:

Build / JDK 8 and Scala 2.12 / kafka.network.ConnectionQuotasTest.testListenerConnectionRateLimitWhenActualRateAboveLimit()

@showuon showuon merged commit a673f21 into apache:trunk Apr 29, 2022
@showuon
Copy link
Copy Markdown
Member

showuon commented Apr 29, 2022

@karuturi , thanks for the contribution!

@karuturi
Copy link
Copy Markdown
Member Author

Thanks for your time @showuon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants