KAFKA-18619: New consumer topic metadata events should set requireMetadata flag by frankvicky · Pull Request #18668 · apache/kafka

frankvicky · 2025-01-22T15:23:05Z

JIRA: KAFKA-18619
In short, the new async consumer's topic metadata operations are unwared of metadata errors because of not overriding requireSubscriptionMetadata.
For further details, please refer to the jira ticket.

Committer Checklist (excluded from commit message)

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

frankvicky · 2025-01-22T15:27:28Z

Hi @lianetm,

I have enabled the SaslClientsWithInvalidCredentialsTest.scala for the new consumer with this patch. Most of the tests pass, except for testConsumerWithAuthenticationFailure. This test fails due to a retry assertion (expected 1 but got 0).

However, I believe this is a separate issue that needs investigation and is not related to this patch.

lianetm · 2025-01-22T17:29:55Z

Thanks for the patch @frankvicky! Could you enable here all the tests that pass so we can have validation of the change? We can investigate a bit the one that still fails, if it's related to another issue we can track and address it separately. Makes sense?

frankvicky · 2025-01-23T00:14:47Z

+  private def verifyWithRetryPredicate(predicate: => Boolean): Unit = {
+    var attempts = 0
+    TestUtils.waitUntilTrue(() => {
+      try {
+        attempts += 1
+        predicate
+      } catch {
+        case _: SaslAuthenticationException => false
+      }
+    }, s"Operation did not succeed within timeout after $attempts")
+  }


Hi @lianetm
I have found that the cause of testConsumerWithAuthenticationFailure failing is due to a non-retriable assertion.

kafka/core/src/test/scala/integration/kafka/api/SaslClientsWithInvalidCredentialsTest.scala

Line 157 in 94a1bfb

verifyWithRetry(assertEquals(1, consumer.poll(Duration.ofMillis(1000)).count))

If we pass the assertion as an argument to verifyWithRetry, it will fail because we don't catch AssertionFailedError (and we shouldn't do it). The subscribe call needs to wait for metadata updates and it's not as quick as assign, so there's a high chance it will fail on the first poll. As mentioned, when we pass the assertion as an argument, it will fail with AssertionFailedError and cannot retry.

interesting, makes sense to me.

But then, couldn't we just change the

verifyWithRetry(assertEquals(1, consumer.poll(Duration.ofMillis(1000)).count))

to

verifyWithRetry(consumer.poll(Duration.ofMillis(1000)).count == 1)

(just to avoid introducing this new verifyWithRetryPredicate)

Sure, i'll give it a try

lianetm

Thanks for the updates @frankvicky !

lianetm · 2025-01-23T18:47:14Z

  @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumAndGroupProtocolNames)
-  @MethodSource(Array("getTestQuorumAndGroupProtocolParametersClassicGroupProtocolOnly"))
+  @MethodSource(Array("getTestQuorumAndGroupProtocolParametersAll"))
  def testTransactionalProducerWithAuthenticationFailure(quorum: String, groupProtocol: String): Unit = {


Looks like it does not use a consumer. Could you double check and if so, then no need to run it for both consumers.

should we better remove the groupProtocol param given that it does not apply here? (and parametrize the test with the quorum only)

@ParameterizedTest @ValueSource(strings = Array("kraft"))

I got your point 😄

lianetm · 2025-01-23T18:48:03Z

  @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumAndGroupProtocolNames)
-  @MethodSource(Array("getTestQuorumAndGroupProtocolParametersClassicGroupProtocolOnly"))
+  @MethodSource(Array("getTestQuorumAndGroupProtocolParametersAll"))
  def testKafkaAdminClientWithAuthenticationFailure(quorum: String, groupProtocol: String): Unit = {


similar as above, looks like it does need to run for both consumers?

lianetm · 2025-01-23T19:18:30Z

+  private def verifyWithRetryPredicate(predicate: => Boolean): Unit = {
+    var attempts = 0
+    TestUtils.waitUntilTrue(() => {
+      try {
+        attempts += 1
+        predicate
+      } catch {
+        case _: SaslAuthenticationException => false
+      }
+    }, s"Operation did not succeed within timeout after $attempts")
+  }


interesting, makes sense to me.

But then, couldn't we just change the

verifyWithRetry(assertEquals(1, consumer.poll(Duration.ofMillis(1000)).count))

to

verifyWithRetry(consumer.poll(Duration.ofMillis(1000)).count == 1)

(just to avoid introducing this new verifyWithRetryPredicate)

lianetm

Thanks for the updates!

lianetm · 2025-01-24T15:49:23Z

  @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumAndGroupProtocolNames)
-  @MethodSource(Array("getTestQuorumAndGroupProtocolParametersClassicGroupProtocolOnly"))
+  @MethodSource(Array("getTestQuorumAndGroupProtocolParametersAll"))
  def testTransactionalProducerWithAuthenticationFailure(quorum: String, groupProtocol: String): Unit = {


should we better remove the groupProtocol param given that it does not apply here? (and parametrize the test with the quorum only)

@ParameterizedTest @ValueSource(strings = Array("kraft"))

lianetm · 2025-01-24T15:49:49Z

  @ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumAndGroupProtocolNames)
-  @MethodSource(Array("getTestQuorumAndGroupProtocolParametersClassicGroupProtocolOnly"))
+  @MethodSource(Array("getTestQuorumAndGroupProtocolParametersAll"))
  def testKafkaAdminClientWithAuthenticationFailure(quorum: String, groupProtocol: String): Unit = {


lianetm

Thanks! LGTM. I just re-triggered the build to see if we can get rid of some of the unrelated failures that have been fixed recently.

…adata flag

frankvicky · 2025-01-29T05:55:48Z

Failed test is handled by #18735

…adata flag (#18668) Reviewers: Lianet Magrans <lmagrans@confluent.io>

lianetm · 2025-01-29T14:01:04Z

Merged to trunk and cherry-picked to 4.0 52280cd

…ibrdkafka-compressed-produce-fails * apache-github/trunk: MINOR: prevent exception from HdrHistogram (apache#18674) KAFKA-18653: Fix mocks and potential thread leak issues causing silent RejectedExecutionException in share group broker tests (apache#18725) KAFKA-18646: Null records in fetch response breaks librdkafka (apache#18726) KAFKA-18619: New consumer topic metadata events should set requireMetadata flag (apache#18668) KAFKA-18488: Improve KafkaShareConsumerTest (apache#18728)

…adata flag (apache#18668) Reviewers: Lianet Magrans <lmagrans@confluent.io>

github-actions Bot added triage PRs from the community consumer clients small Small PRs labels Jan 22, 2025

chia7712 added the ci-approved label Jan 22, 2025

frankvicky force-pushed the KAFKA-18619 branch from 2446388 to 4c1f6dd Compare January 23, 2025 00:02

github-actions Bot added core Kafka Broker tools labels Jan 23, 2025

frankvicky commented Jan 23, 2025

View reviewed changes

github-actions Bot removed the triage PRs from the community label Jan 23, 2025

lianetm reviewed Jan 23, 2025

View reviewed changes

frankvicky force-pushed the KAFKA-18619 branch 2 times, most recently from e7ba1be to d82baf5 Compare January 24, 2025 04:30

lianetm reviewed Jan 24, 2025

View reviewed changes

frankvicky force-pushed the KAFKA-18619 branch 3 times, most recently from 6955c82 to adeb17d Compare January 28, 2025 05:23

frankvicky mentioned this pull request Jan 28, 2025

KAFKA-18527: SaslClientsWithInvalidCredentialsTest.java should run for async consumer #18609

Closed

3 tasks

frankvicky force-pushed the KAFKA-18619 branch from adeb17d to 952599b Compare January 28, 2025 08:48

lianetm approved these changes Jan 28, 2025

View reviewed changes

KAFKA-18619: New consumer topic metadata events should set requireMet…

7acaa1d

…adata flag

frankvicky force-pushed the KAFKA-18619 branch from 952599b to 7acaa1d Compare January 29, 2025 02:55

lianetm merged commit 97a2280 into apache:trunk Jan 29, 2025

lianetm pushed a commit that referenced this pull request Jan 29, 2025

KAFKA-18619: New consumer topic metadata events should set requireMet…

52280cd

…adata flag (#18668) Reviewers: Lianet Magrans <lmagrans@confluent.io>

pdruley pushed a commit to pdruley/kafka that referenced this pull request Feb 12, 2025

KAFKA-18619: New consumer topic metadata events should set requireMet…

88b42fa

…adata flag (apache#18668) Reviewers: Lianet Magrans <lmagrans@confluent.io>

manoj-mathivanan pushed a commit to manoj-mathivanan/kafka that referenced this pull request Feb 19, 2025

KAFKA-18619: New consumer topic metadata events should set requireMet…

003fdff

…adata flag (apache#18668) Reviewers: Lianet Magrans <lmagrans@confluent.io>

Conversation

frankvicky commented Jan 22, 2025

Committer Checklist (excluded from commit message)

Uh oh!

frankvicky commented Jan 22, 2025

Uh oh!

lianetm commented Jan 22, 2025

Uh oh!

frankvicky Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lianetm Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lianetm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lianetm Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lianetm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lianetm left a comment

Choose a reason for hiding this comment

Uh oh!

frankvicky commented Jan 29, 2025

Uh oh!

lianetm commented Jan 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

frankvicky Jan 23, 2025 •

edited

Loading

lianetm Jan 23, 2025 •

edited

Loading

lianetm Jan 23, 2025 •

edited

Loading