Skip to content

Conversation

@bboyleonp666
Copy link
Contributor

TestUtils.verifyNoUnexpectedThreads() will verify there's no remaining threads that might affect the consequent test cases, which should be checked before and after all test cases.

Committer Checklist (excluded from commit message)

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

@ClusterTest(types = {Type.ZK, Type.KRAFT, Type.CO_KRAFT}, disksPerBroker = 2),
})
public void testClusterTestWithDisksPerBroker() throws ExecutionException, InterruptedException {
Admin admin = clusterInstance.createAdminClient();
Copy link
Member

Choose a reason for hiding this comment

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

Currently, all admins created by clusterInstance.createAdminClient are stored in a collection. They will get closed after tests, so we don't need to close they manually. I know that is a bit weird to IDE, but that is a style in code base so we can keep it as it is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed. Thanks!

@chia7712
Copy link
Member

please fix the style error

@gharris1727
Copy link
Contributor

#15101 was the previous iteration of this, please see my concerns raised in that PR.

@chia7712
Copy link
Member

#15101 was the previous iteration of this, please see my concerns raised in that PR.

happy to know there was a PR for it. I read the comments, and it seems we can use BeforeEachCallback/AfterEachCallback to reduce the scope to the method-only enforcement. This solution works only for new test infra, but it should be fine as eventually we will be there. WDYT?

@gharris1727
Copy link
Contributor

This solution works only for new test infra, but it should be fine as eventually we will be there. WDYT?

This is actually a good thing, because I don't like the cascading failure property of verifyNoUnexpectedThreads, and I think it should be replaced with an alternative without that property. This is applying it in ~45 new places that may not have been affected by the cascading failure previously.

If this brings us closer to removing verifyNoUnexpectedThreads then i'm fine with it.

@chia7712
Copy link
Member

I have discussed with @bboyleonp666, and he has no bandwidth to address requests. Hence, I'm about to close this PR.

@chia7712 chia7712 closed this Jun 27, 2024
@bboyleonp666 bboyleonp666 deleted the bboyleonp/KAFKA-16791 branch July 20, 2024 01:01
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.

4 participants