Skip to content

KAFKA-5235: GetOffsetShell: support for multiple topics and consumer configuration override#9430

Merged
dajac merged 7 commits intoapache:trunkfrom
urbandan:KIP-635_GetOffsetShell
Feb 11, 2021
Merged

KAFKA-5235: GetOffsetShell: support for multiple topics and consumer configuration override#9430
dajac merged 7 commits intoapache:trunkfrom
urbandan:KIP-635_GetOffsetShell

Conversation

@urbandan
Copy link
Copy Markdown
Contributor

@urbandan urbandan commented Oct 14, 2020 •
edited
Loading

Implements KIP-635

Changes:

  • Added kafka-get-offsets.sh script
  • Removed deprecated max-wait-ms and offsets arguments
  • Updated tool to query all topic-partitions by default
  • Updated topic argument to support patterns
  • Added topic-partitions argument to support a list of topic-partition patterns
  • Added exclude-internal-topics to support filtering internal topics

Testing done: added new ducktape tests for the tool.

Committer Checklist (excluded from commit message)

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

@urbandan urbandan changed the title Kip 635 get offset shell KAFKA-5235: GetOffsetShell: support for multiple topics and consumer configuration override Oct 14, 2020
@dajac
Copy link
Copy Markdown
Member

dajac commented Oct 20, 2020

@urbandan Thanks for the PR. I haven't looked at it yet but I have already noticed that there are no unit/integration tests. We need some. You can look at how other commands are tested. Having ducktape tests is very good but not enough to catch bugs and regressions.

@ijuma ijuma requested a review from dajac October 22, 2020 21:02
@dajac dajac mentioned this pull request Oct 24, 2020
Closed
@urbandan
Copy link
Copy Markdown
Contributor Author

urbandan commented Nov 10, 2020

@dajac Thanks for the reminder, pushed a bunch of unit tests covering the GetOffsetShell tool.

@urbandan urbandan force-pushed the KIP-635_GetOffsetShell branch from c8f317e to 512461e Compare November 19, 2020 10:33
dajac
dajac reviewed Jan 15, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@dajac dajac left a comment

Choose a reason for hiding this comment

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

@urbandan Thanks for the PR and sorry for the delay. I have made a first pass and have left some comments.

Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/test/scala/kafka/tools/GetOffsetShellTest.scala Outdated
Comment thread core/src/test/scala/kafka/tools/GetOffsetShellTest.scala Outdated
Comment thread core/src/test/scala/kafka/tools/GetOffsetShellTest.scala Outdated
Comment thread core/src/test/scala/kafka/tools/GetOffsetShellTest.scala Outdated
Comment thread core/src/test/scala/kafka/tools/GetOffsetShellTest.scala Outdated
@urbandan
Copy link
Copy Markdown
Contributor Author

urbandan commented Jan 25, 2021

@dajac Thank you for your review. Managed to work on this, and addressed your points.

@dajac
Copy link
Copy Markdown
Member

dajac commented Jan 25, 2021

@urbandan Thanks for the update. I will make another pass on it this week. It seems that there are compilation error. Could you take a look please?

@urbandan urbandan force-pushed the KIP-635_GetOffsetShell branch 2 times, most recently from a9f7eaf to 446a9bf Compare January 26, 2021 08:09
@urbandan
Copy link
Copy Markdown
Contributor Author

urbandan commented Jan 26, 2021

@dajac Fixed the compilation issues - there is a test failure, but I don't think those are related to the change

dajac
dajac reviewed Jan 27, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@dajac dajac left a comment

Choose a reason for hiding this comment

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

@urbandan Thanks for the updates. I have left few more comments.

Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/test/scala/kafka/tools/GetOffsetShellTest.scala Outdated
@dajac
Copy link
Copy Markdown
Member

dajac commented Jan 27, 2021

Could you rebase the PR as well?

@urbandan urbandan force-pushed the KIP-635_GetOffsetShell branch from 446a9bf to d5b6f5e Compare February 3, 2021 15:09
@urbandan
Copy link
Copy Markdown
Contributor Author

urbandan commented Feb 3, 2021

@dajac Thank you for your comments, finally managed to address them. Also rebased the branch.

@urbandan urbandan force-pushed the KIP-635_GetOffsetShell branch from d5b6f5e to 2b1cc08 Compare February 8, 2021 08:24
dajac
dajac reviewed Feb 8, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@dajac dajac left a comment

Choose a reason for hiding this comment

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

@urbandan Thanks for the updates. I left few more comments.

Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
dajac
dajac reviewed Feb 9, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@dajac dajac left a comment

Choose a reason for hiding this comment

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

@urbandan Thanks for the updates. We are almost there! I just left few minor/cosmetic comments.

Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
Comment thread core/src/main/scala/kafka/tools/GetOffsetShell.scala Outdated
@dajac
Copy link
Copy Markdown
Member

dajac commented Feb 9, 2021

Launched system tests: https://jenkins.confluent.io/job/system-test-kafka-branch-builder/4362/

@urbandan urbandan force-pushed the KIP-635_GetOffsetShell branch from 8e69e1d to d3d8115 Compare February 10, 2021 11:49
@urbandan
Copy link
Copy Markdown
Contributor Author

urbandan commented Feb 10, 2021

@dajac Really appreciate your review, fixed the last couple of comments.

@dajac
Copy link
Copy Markdown
Member

dajac commented Feb 10, 2021

@urbandan It seems that most of the get offset system tests failed: http://confluent-kafka-branch-builder-system-test-results.s3-us-west-2.amazonaws.com/2021-02-09--001.1612926625--urbandan--KIP-635_GetOffsetShell--8e69e1dc5/report.html. Could you take a look to see what went wrong?

@urbandan urbandan force-pushed the KIP-635_GetOffsetShell branch from d3d8115 to 324810b Compare February 10, 2021 12:14
@urbandan urbandan force-pushed the KIP-635_GetOffsetShell branch from 324810b to fabd539 Compare February 10, 2021 14:12
@urbandan
Copy link
Copy Markdown
Contributor Author

urbandan commented Feb 10, 2021

@dajac I think I found the issue, pushed the fix. There was a typo in the command arg names. Could not get to execute the tests locally yet.

@dajac
Copy link
Copy Markdown
Member

dajac commented Feb 10, 2021

Triggered another system test: https://jenkins.confluent.io/job/system-test-kafka-branch-builder/4364/

@dajac
Copy link
Copy Markdown
Member

dajac commented Feb 11, 2021

All the kafkatest.tests.core.get_offset_shell_test system tests passed.

dajac
dajac approved these changes Feb 11, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@dajac dajac left a comment

Choose a reason for hiding this comment

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

LGTM! @urbandan Thanks for your contribution!

@dajac
Copy link
Copy Markdown
Member

dajac commented Feb 11, 2021

Failed tests are not related. Merging to trunk.

@dajac dajac merged commit 202ff63 into apache:trunk Feb 11, 2021
ijuma added a commit to ijuma/kafka that referenced this pull request Feb 14, 2021
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into reduce-produc…
d7c43a1 
…e-allocations-lz4

* apache-github/trunk: (118 commits)
  KAFKA-12327: Remove MethodHandle usage in CompressionType (apache#10123)
KAFKA-12297: Make MockProducer return RecordMetadata with values as
per contract
  MINOR: Update zstd and use classes with no finalizers (apache#10120)
KAFKA-12326: Corrected regresion in MirrorMaker 2 executable
introduced with KAFKA-10021 (apache#10122)
KAFKA-12321 the comparison function for uuid type should be 'equals'
rather than '==' (apache#10098)
  MINOR: Add FetchSnapshot API doc in KafkaRaftClient (apache#10097)
  MINOR: KIP-631 KafkaConfig fixes and improvements (apache#10114)
  KAFKA-12272: Fix commit-interval metrics (apache#10102)
  MINOR: Improve confusing admin client shutdown logging (apache#10107)
  MINOR: Add BrokerMetadataListener (apache#10111)
  MINOR: Support Raft-based metadata quorums in system tests (apache#10093)
MINOR: add the MetaLogListener, LocalLogManager, and Controller
interface. (apache#10106)
  MINOR: Introduce the KIP-500 Broker lifecycle manager (apache#10095)
MINOR: Remove always-passing validation in
TestRecordTest#testProducerRecord (apache#9930)
KAFKA-5235: GetOffsetShell: Support for multiple topics and consumer
configuration override (KIP-635) (apache#9430)
MINOR: Prevent creating partition.metadata until ID can be written
(apache#10041)
  MINOR: Add RaftReplicaManager (apache#10069)
MINOR: Add ClientQuotaMetadataManager for processing QuotaRecord
(apache#10101)
  MINOR: Rename DecommissionBrokers to UnregisterBrokers (apache#10084)
MINOR: KafkaBroker.brokerState should be volatile instead of
AtomicReference (apache#10080)
  ...

clients/src/main/java/org/apache/kafka/common/record/CompressionType.java
core/src/test/scala/unit/kafka/coordinator/group/GroupMetadataManagerTest.scala
This was referenced Dec 17, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dajac dajac dajac 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.

2 participants

@urbandan @dajac