Skip to content

KAFKA-12989: MockClient should respect the request matcher passed to prepareUnsupportedVersionResponse #10980

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

Closed
wants to merge 3 commits into from
Closed

KAFKA-12989: MockClient should respect the request matcher passed to prepareUnsupportedVersionResponse #10980

wants to merge 3 commits into from

Conversation

kirktrue
Copy link
Contributor

@kirktrue kirktrue commented Jul 6, 2021

Handle the case where matches returns false and throw the InvalidStateException as stated by the JavaDoc.

We need to guard against this unexpected runtime error in the KafkaAdminClient's sendEligibleCalls method with a try/catch. Not 100% sure if that's kosher or not.

Included a targeted unit test for this case. The remaining tests in KafkaAdminTestClient continue to pass.

Committer Checklist (excluded from commit message)

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

Kirk True added 2 commits July 6, 2021 11:36
…prepareUnsupportedVersionResponse

Handle the case where matches returns false and throw the
InvalidStateException as stated by the JavaDoc.

We need to guard against this unexpected runtime error in the
KafkaAdminClient's sendEligibleCalls method with a try/catch. Not 100%
sure if that's kosher or not.
@kirktrue
Copy link
Contributor Author

kirktrue commented Jul 6, 2021

@hachikuji - would you be willing to assign a reviewer for this PR?

The failing tests look like they're related to KRaft tests, so I don't think they're related.

@kirktrue
Copy link
Contributor Author

kirktrue commented Jul 8, 2021

@cmccabe - could you take a look at this and/or assign a reviewer for this? Thanks!

@cmccabe
Copy link
Contributor

cmccabe commented Jul 26, 2021

Why is NetworkClient#send throwing an exception? It shouldn't be doing that, right? Can you explain more about the problem that this PR fixes?

@kirktrue
Copy link
Contributor Author

kirktrue commented Jul 27, 2021

Why is NetworkClient#send throwing an exception? It shouldn't be doing that, right? Can you explain more about the problem that this PR fixes?

Per the original issue the MockClient used for testing allows for fault injection via the RequestMatcher. If the test sets up the condition where the request doesn't match some condition, the MockClient.send method is supposed to throw an IllegalStateException.

That change seemed straightforward except that this now caused problems in KafkaAdminClient. Because it's not expecting any errors, this exception causes the thread in KafkaAdminClient.sendEligibleCalls that is servicing requests to die, hence my addition of the try/catch wrapper.

That said, I'm not 100% confident that this change is the right way to handle things. Please advise.

Thanks!

@kirktrue kirktrue closed this Oct 3, 2023
@kirktrue kirktrue deleted the KAFKA-12989-throw-invalid-state-exception-if-matches-is-false branch October 25, 2023 00:16
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.

2 participants