-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
KAFKA-14596: Move TopicCommand to tools #13201
Conversation
1c25106
to
d7c52df
Compare
This PR is being marked as stale since it has not had any activity in 90 days. If you would like to keep this PR alive, please ask a committer for review. If the PR has merge conflicts, please update it with the latest from trunk (or appropriate release branch) |
@OmniaGM can you please rebase this with trunk and I would be happy to begin a review for this. |
b574ef2
to
68e39c0
Compare
Hi @divijvaidya I just did rebase this with trunk |
Hi @OmniaGM.
I think #13158 is ready. I'll try to drive some attention to it.
I looks like |
Will update this PR once these prs are merged. |
I have updated the PR to use #13158 and #13278. The test checks are failing because of an unrelated class. However the Edit: Actually running Out of curiosity are we planning to move |
d126a39
to
993b6d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @OmniaGM, thanks. I left some comments.
Additionally, this error message is different from previous implementation:
### OLD
$ bin/kafka-topics.sh --bootstrap-server :9092 --describe --topic my-topic-foo --delete-config sdsad
Option "[delete-config]" can't be used with option "[describe]"
### NEW
$ bin/kafka-topics.sh --bootstrap-server :9092 --describe --topic my-topic-foo --delete-config sdsad
Option "[delete-config]" can't be used with option "[bootstrap-server]"
07eaec5
to
81a0ef8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @OmniaGM, there is still one formatting issue in the first output line:
$ bin/kafka-topics.sh --bootstrap-server :9092 --describe --topic my-topic
Topic: my-topic TopicId: CITkda1vT5i6T7GdwrWCdA PartitionCount: %s3 ReplicationFactor: %s2 Configs: %smin.insync.replicas=1
Topic: my-topic Partition: 0 Leader: 3 Replicas: 3,4 Isr: 3,4
Topic: my-topic Partition: 1 Leader: 4 Replicas: 4,2 Isr: 4,2
Topic: my-topic Partition: 2 Leader: 2 Replicas: 2,3 Isr: 2,3
After rebasing on your branch, I'm getting the following exception on the integration tests with quorum=kraft:
java.lang.NoClassDefFoundError: org/apache/kafka/server/log/remote/storage/RemoteStorageException
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @OmniaGM, I think we are almost there.
Thanks for the effort of finding a workaround for the dependency issue. Well done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks.
@OmniaGM Thanks for the PR. Can you rebase on trunk to resolve the conflicts? |
Updated the pr waiting now for the pipeline to pass |
@OmniaGM Thanks for the update. I've not had time to take a look yet but noticed there's a compilation failure:
|
@mimaison fixed the compilation issue. I spotted another one with Java21 and fixed it as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, it wasn't an easy one to convert! It looks good overall, I left a few comments and suggestions.
tools/src/test/java/org/apache/kafka/tools/TopicCommandIntegrationTest.java
Outdated
Show resolved
Hide resolved
tools/src/test/java/org/apache/kafka/tools/TopicCommandIntegrationTest.java
Outdated
Show resolved
Hide resolved
tools/src/test/java/org/apache/kafka/tools/TopicCommandTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! I left a few more small suggestions
@OmniaGM thanks for the updates. I'm getting a test failure in TopicCommandIntegrationTest:
|
Hi @mimaison I think this is flaky test I added |
With the latest updates it's not failing anymore on my laptop. I'll let the Apache CI run before merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM
Reviewers: Mickael Maison <mickael.maison@gmail.com>, Federico Valeri <fedevaleri@gmail.com>
This pr include
Some implementation notes:
ToolsTestUtils.createBrokerProperties
as a wrapper forTestUtils.createBrokerConfig
to avoid converting betweenMap<Integer, String>
andMap<Int, String>
.TestUtils.setReplicationThrottleForPartitions
andTestUtils.removeReplicationThrottleForPartitions
toToolsTestUtils
as the methods are used onlyTopicCommandIntegrationTest
andReassignPartitionsIntegrationTest
. We need to remove it fromTestUtils
once we migrateReassignPartitions
TestInfoUtils.TestWithParameterizedQuorumName
toToolsTestUtils
as java convert this into a getter function which cannot be used withParameterizedTest
CoreUtils.duplicate
toToolsUtils
MoveUsed KAFKA-14647: Moving TopicFilter to server-common/utils #13158TopicFilter
out of core. (This is part of https://issues.apache.org/jira/browse/KAFKA-14647 which seems not moving for a while )DuplicateUsed the new exceptions from KAFKA-14591 DeleteRecordsCommand moved to tools #13278AdminCommandFailedException
andAdminOperationException
out of core.More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.
Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.
Committer Checklist (excluded from commit message)