Skip to content
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.

Sign up for GitHub

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

Jump to bottom

KAFKA-9507 AdminClient should check for missing committed offsets #8057

Merged
merged 2 commits into from
Feb 8, 2020
Merged

KAFKA-9507 AdminClient should check for missing committed offsets #8057

merged 2 commits into from
Feb 8, 2020

Conversation

splett2
Copy link
Contributor

@splett2 splett2 commented Feb 6, 2020

JIRA: https://issues.apache.org/jira/browse/KAFKA-9507

Addresses exception being thrown by AdminClient when listConsumerGroupOffsets returns a negative offset.

Committer Checklist (excluded from commit message)

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

hachikuji
hachikuji reviewed Feb 6, 2020
View reviewed changes
Copy link
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

Looks good, just a few minor suggestions.

clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java Outdated
@@ -1502,15 +1503,18 @@ public void testDescribeConsumerGroupOffsets() throws Exception {
Optional.empty(), "", Errors.NONE));
responseData.put(myTopicPartition2, new OffsetFetchResponse.PartitionData(20,
Optional.empty(), "", Errors.NONE));
responseData.put(myTopicPartition3, new OffsetFetchResponse.PartitionData(-1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use OffsetFetchResponse.INVALID_OFFSET

clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java Show resolved Hide resolved
clients/src/main/java/org/apache/kafka/clients/admin/ListConsumerGroupOffsetsResult.java Outdated
@@ -40,6 +40,7 @@

/**
* Return a future which yields a map of topic partitions to OffsetAndMetadata objects.
* If the partition does not have a committed offset, the corresponding value will be null
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest rephrasing this:

If the group does not have a committed offset for this partition, the corresponding value in the returned map will be null.

@splett2
Copy link
Contributor Author

splett2 commented Feb 7, 2020

Pushed changes to address review comments.

@hachikuji
Copy link
Contributor

ok to test

@hachikuji
Copy link
Contributor

retest this please

@splett2
Copy link
Contributor Author

splett2 commented Feb 8, 2020

Looks like the test that failed this time is a known flaky test
https://issues.apache.org/jira/browse/KAFKA-8940

hachikuji
hachikuji approved these changes Feb 8, 2020
View reviewed changes
Copy link
Contributor

@hachikuji hachikuji 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!

@hachikuji hachikuji merged commit 7a2a198 into apache:trunk Feb 8, 2020
hachikuji pushed a commit that referenced this pull request Feb 8, 2020
@splett2 @hachikuji
KAFKA-9507; AdminClient should check for missing committed offsets (#…
64f8023 
…8057)

Addresses exception being thrown by `AdminClient` when `listConsumerGroupOffsets` returns a negative offset. A negative offset indicates the absence of a committed offset for a requested partition, and should result in a null in the returned offset map.

Reviewers: Anna Povzner <anna@confluent.io>, Jason Gustafson <jason@confluent.io>
hachikuji pushed a commit that referenced this pull request Feb 8, 2020
@splett2 @hachikuji
KAFKA-9507; AdminClient should check for missing committed offsets (#…
2e00ecf 
…8057)

Addresses exception being thrown by `AdminClient` when `listConsumerGroupOffsets` returns a negative offset. A negative offset indicates the absence of a committed offset for a requested partition, and should result in a null in the returned offset map.

Reviewers: Anna Povzner <anna@confluent.io>, Jason Gustafson <jason@confluent.io>
hachikuji pushed a commit that referenced this pull request Feb 8, 2020
@splett2 @hachikuji
KAFKA-9507; AdminClient should check for missing committed offsets (#…
33e899d 
…8057)

Addresses exception being thrown by `AdminClient` when `listConsumerGroupOffsets` returns a negative offset. A negative offset indicates the absence of a committed offset for a requested partition, and should result in a null in the returned offset map.

Reviewers: Anna Povzner <anna@confluent.io>, Jason Gustafson <jason@confluent.io>
rite2nikhil pushed a commit to confluentinc/kafka that referenced this pull request Feb 12, 2020
@splett2 @rite2nikhil
KAFKA-9507; AdminClient should check for missing committed offsets (a…
586a7d9 
…pache#8057)

Addresses exception being thrown by `AdminClient` when `listConsumerGroupOffsets` returns a negative offset. A negative offset indicates the absence of a committed offset for a requested partition, and should result in a null in the returned offset map.

Reviewers: Anna Povzner <anna@confluent.io>, Jason Gustafson <jason@confluent.io>
stanislavkozlovski pushed a commit to stanislavkozlovski/kafka that referenced this pull request Feb 18, 2020
@splett2 @stanislavkozlovski
KAFKA-9507; AdminClient should check for missing committed offsets (a…
e4ba3f4 
…pache#8057)

Addresses exception being thrown by `AdminClient` when `listConsumerGroupOffsets` returns a negative offset. A negative offset indicates the absence of a committed offset for a requested partition, and should result in a null in the returned offset map.

Reviewers: Anna Povzner <anna@confluent.io>, Jason Gustafson <jason@confluent.io>
ijuma added a commit to ijuma/kafka that referenced this pull request Apr 28, 2020
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into skip-to-struc…
186aad4 
…t-for-generated-requests

* apache-github/trunk: (410 commits)
  KAFKA-8843: KIP-515: Zookeeper TLS support
  MINOR: Add missing quote for malformed line content (apache#8070)
  MINOR: Simplify KafkaProducerTest (apache#8044)
  KAFKA-9507; AdminClient should check for missing committed offsets (apache#8057)
  KAFKA-9519: Deprecate the --zookeeper flag in ConfigCommand (apache#8056)
  KAFKA-9509; Fixing flakiness of MirrorConnectorsIntegrationTest.testReplication (apache#8048)
  HOTFIX: Fix two test failures in JDK11 (apache#8063)
  DOCS - clarify transactionalID and idempotent behavior (apache#7821)
  MINOR: further InternalTopologyBuilder cleanup  (apache#8046)
  MINOR: Add timer for update limit offsets (apache#8047)
  HOTFIX: Fix spotsbug failure in Kafka examples (apache#8051)
  KAFKA-9447: Add new customized EOS model example (apache#8031)
  KAFKA-8164: Add support for retrying failed (apache#8019)
  HOTFIX: checkstyle for newly added unit test
  KAFKA-9261; Client should handle unavailable leader metadata (apache#7770)
  MINOR: Fix typos introduced in KIP-559 (apache#8042)
  MINOR: Fixing null handilg in ValueAndTimestampSerializer (apache#7679)
  KAFKA-9113: Clean up task management and state management (apache#7997)
  MINOR: fix checkstyle issue in ConsumerConfig.java (apache#8038)
  KAFKA-9491; Increment high watermark after full log truncation (apache#8037)
  ...
qq619618919 pushed a commit to qq619618919/kafka that referenced this pull request May 12, 2020
@splett2 @qq619618919
KAFKA-9507; AdminClient should check for missing committed offsets (a…
797996f 
…pache#8057)

Addresses exception being thrown by `AdminClient` when `listConsumerGroupOffsets` returns a negative offset. A negative offset indicates the absence of a committed offset for a requested partition, and should result in a null in the returned offset map.

Reviewers: Anna Povzner <anna@confluent.io>, Jason Gustafson <jason@confluent.io>
@IgnacioAcunaF IgnacioAcunaF mentioned this pull request Jun 10, 2021
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apovzner apovzner apovzner left review comments

@hachikuji hachikuji hachikuji approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@splett2 @hachikuji @apovzner