Skip to content

KAFKA-17960; PlaintextAdminIntegrationTest.testConsumerGroups fails with CONSUMER group protocol#18234

Merged
dajac merged 3 commits intoapache:trunkfrom
dajac:KAFKA-17960
Dec 17, 2024
Merged

KAFKA-17960; PlaintextAdminIntegrationTest.testConsumerGroups fails with CONSUMER group protocol#18234
dajac merged 3 commits intoapache:trunkfrom
dajac:KAFKA-17960

Conversation

@dajac
Copy link
Copy Markdown
Member

@dajac dajac commented Dec 17, 2024

The issue has been fixed by https://issues.apache.org/jira/browse/KAFKA-18188. We can re-enable the test with the CONSUMER protocol.

Committer Checklist (excluded from commit message)

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

@github-actions github-actions bot added core Kafka Broker tests Test fixes (including flaky tests) small Small PRs labels Dec 17, 2024
lianetm
lianetm reviewed Dec 17, 2024
View reviewed changes
Comment thread core/src/test/scala/integration/kafka/server/QuorumTestHarness.scala

// The following parameter groups are to *temporarily* avoid bugs with the CONSUMER group protocol Consumer
// implementation that would otherwise cause tests to fail.
def getTestQuorumAndGroupProtocolParametersClassicGroupProtocolOnly_KAFKA_17960: stream.Stream[Arguments] = getTestQuorumAndGroupProtocolParametersClassicGroupProtocolOnly
Copy link
Copy Markdown
Member

@lianetm lianetm Dec 17, 2024

Choose a reason for hiding this comment

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

there is another test still disabled using this

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

Line 2170 in 9afb274

@MethodSource(Array("getTestQuorumAndGroupProtocolParametersClassicGroupProtocolOnly_KAFKA_17960"))
, I believe it was passing locally so let's just double check and maybe we can enable it too? (or leave this)

Copy link
Copy Markdown
Member Author

@dajac dajac Dec 17, 2024

Choose a reason for hiding this comment

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

Good catch!

Copy link
Copy Markdown
Member Author

@dajac dajac Dec 17, 2024

Choose a reason for hiding this comment

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

I changed the other test too. It passes a few times locally.

lianetm
lianetm approved these changes Dec 17, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@lianetm lianetm left a comment

Choose a reason for hiding this comment

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

Thanks! Let's check it all passes here and with that LGTM

AndrewJSchofield
AndrewJSchofield requested changes Dec 17, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@AndrewJSchofield AndrewJSchofield left a comment

Choose a reason for hiding this comment

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

lgtm, but a little more trivial tidying up seems appropriate.

I have run these tests locally repeatedly with error. The change looks good to me.

Comment thread core/src/test/scala/integration/kafka/server/QuorumTestHarness.scala Outdated
// The following parameter groups are to *temporarily* avoid bugs with the CONSUMER group protocol Consumer
// implementation that would otherwise cause tests to fail.
def getTestQuorumAndGroupProtocolParametersClassicGroupProtocolOnly_KAFKA_17960: stream.Stream[Arguments] = getTestQuorumAndGroupProtocolParametersClassicGroupProtocolOnly
def getTestQuorumAndGroupProtocolParametersClassicGroupProtocolOnly_KAFKA_17961: stream.Stream[Arguments] = getTestQuorumAndGroupProtocolParametersClassicGroupProtocolOnly
Copy link
Copy Markdown
Member

@AndrewJSchofield AndrewJSchofield Dec 17, 2024

Choose a reason for hiding this comment

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

This can also be removed. And the next one. Only the final of these 4 definitions is used after this change.

Copy link
Copy Markdown
Member Author

@dajac dajac Dec 17, 2024

Choose a reason for hiding this comment

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

Oh, so only getTestQuorumAndGroupProtocolParametersClassicGroupProtocolOnly_KAFKA_18034 is still used.

Copy link
Copy Markdown
Member Author

@dajac dajac Dec 17, 2024

Choose a reason for hiding this comment

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

I just removed the unused ones.

AndrewJSchofield
AndrewJSchofield approved these changes Dec 17, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@AndrewJSchofield AndrewJSchofield left a comment

Choose a reason for hiding this comment

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

lgtm. Tests all passing locally for me too.

@dajac dajac merged commit dfc07e0 into apache:trunk Dec 17, 2024
@dajac dajac deleted the KAFKA-17960 branch December 17, 2024 19:09
dajac added a commit that referenced this pull request Dec 17, 2024
@dajac
KAFKA-17960; PlaintextAdminIntegrationTest.testConsumerGroups fails w…
0c433a2 
…ith CONSUMER group protocol (#18234)

The issue has been fixed by https://issues.apache.org/jira/browse/KAFKA-18188. We can re-enable the test with the CONSUMER protocol.

Reviewers: Lianet Magrans <lmagrans@confluent.io>, Andrew Schofield <aschofield@confluent.io>
ijuma added a commit to ijuma/kafka that referenced this pull request Dec 19, 2024
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into kip-896-remov…
2d94927 
…e-old-protocol-versions

* apache-github/trunk: (25 commits)
  KAFKA-18270: FindCoordinator v0 incorrectly tagged as deprecated (apache#18262)
  KAFKA-18284: Add group coordinator records for Streams rebalance protocol (apache#18228)
  MINOR: Fix flaky state updater test (apache#18253)
  MINOR: improve StreamsResetter logging (apache#18237)
  KAFKA-18227: Ensure v2 partitions are not added to last transaction during upgrade (apache#18176)
  Add IT for share consumer with duration base offet auto reset (apache#18251)
  KAFKA-18283: Add StreamsGroupDescribe RPC definitions (apache#18230)
  KAFKA-18241: add docs check to CI (apache#18183)
  KAFKA-18223 Improve flaky test report (apache#18212)
  MINOR Remove triage label in nightly job (apache#18147)
  KAFKA-18294 Remove deprecated SourceTask#commitRecord (apache#18260)
  KAFKA-18264 Remove NotLeaderForPartitionException (apache#18211)
  KAFKA-13722: Refactor SerdeGetter (apache#18242)
  KAFKA-18094 Remove deprecated TopicListing(String, Boolean) (apache#18248)
  KAFKA-18282: Add StreamsGroupHeartbeat RPC definitions (apache#18227)
  KAFKA-18026: KIP-1112 migrate KTableSuppressProcessorSupplier (apache#18150)
  KAFKA-18026: transition KTable#filter impl to use processor wrapper (apache#18205)
  KAFKA-18293 Remove `org.apache.kafka.common.security.oauthbearer.secured.OAuthBearerLoginCallbackHandler` and `org.apache.kafka.common.security.oauthbearer.secured.OAuthBearerValidatorCallbackHandler` (apache#18244)
  MINOR: add assertion about groupEpoch and targetAssignmentEpoch to testConsumerGroups (apache#18203)
  KAFKA-17960; PlaintextAdminIntegrationTest.testConsumerGroups fails with CONSUMER group protocol (apache#18234)
  ...
tedyu pushed a commit to tedyu/kafka that referenced this pull request Jan 6, 2025
@dajac @tedyu
KAFKA-17960; PlaintextAdminIntegrationTest.testConsumerGroups fails w…
3c7ab38 
…ith CONSUMER group protocol (apache#18234)

The issue has been fixed by https://issues.apache.org/jira/browse/KAFKA-18188. We can re-enable the test with the CONSUMER protocol.

Reviewers: Lianet Magrans <lmagrans@confluent.io>, Andrew Schofield <aschofield@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AndrewJSchofield AndrewJSchofield AndrewJSchofield approved these changes

@lianetm lianetm lianetm approved these changes

Assignees

No one assigned

Labels

core Kafka Broker small Small PRs tests Test fixes (including flaky tests)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dajac @AndrewJSchofield @lianetm