KAFKA-14593: Move LeaderElectionCommand to tools#13204
KAFKA-14593: Move LeaderElectionCommand to tools#13204mimaison merged 7 commits intoapache:trunkfrom
Conversation
5828376 to
8b8c1f4
Compare
There was a problem hiding this comment.
The direction from previous migrations is that we don't want to depend on core. I see there are a few kafka.* dependencies in LeaderElectionCommand class. Do you think we can also migrate them as part of this PR?
There was a problem hiding this comment.
It would be bit harder to move them to tools. For example the command uses kafka.utils.json which is used by kafka.server, kafka.security.authorizer. And kafka.admin.AdminOperationException is used by kafka.admin, kafka.controller, kafka.zk, etc. So not sure it's easy to move them out.
There was a problem hiding this comment.
These classes may be candidates to land in the module server-common?
There was a problem hiding this comment.
Let's avoid making tools depend on core if possible. I opened https://issues.apache.org/jira/browse/KAFKA-14737 to move the kafka.utils.json classes to server-common.
There was a problem hiding this comment.
I removed the dependencies on the core now. Out of curiosity do we plan to move kafka.utils.TestUtils out of core and maybe into common at some point?
bd21b9d to
817730d
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) |
mimaison
left a comment
There was a problem hiding this comment.
Thanks for the PR. It looks good overall, I left a few suggestions to cleanup some logic.
b2bcafd to
638f48a
Compare
|
The test is failing because of an unrelated package |
|
I've re-kicked a build, hopefully that will clear the test failures. |
|
Still with all the |
I will have a look into the |
|
@mimaison I can't reproduce the problem locally. I ran |
|
To be honest, I'm not sure. Try rebasing on trunk. Maybe there's a conflict with a recent commit. |
e0f51e5 to
926dec0
Compare
|
I managed to reproduce some of the failed tests locally. Seems like any test that includes shutting down a broker and bringing it back up causes an odd situation where the leader of the topic shrinks the ISR to itself even though there is another replica online. So far the only thing I'm noticing is that every time the test restarts a broker it gets these 2 errors and Am not sure what changed but the broker is failing to start |
926dec0 to
867768d
Compare
|
I found out why the tests were failing because we are hitting a similar problem to this gradle issue#847 that causing transitive dependency problems where |
|
The test is failing for different packages https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13204/19/testReport/ and one of the tasks succeeded in building but failed with an error |
mimaison
left a comment
There was a problem hiding this comment.
Thanks for the PR and for figuring out the dependency issue. LGTM
Reviewers: Mickael Maison <mickael.maison@gmail.com>, Federico Valeri <fedevaleri@gmail.com>
Reviewers: Mickael Maison <mickael.maison@gmail.com>, Federico Valeri <fedevaleri@gmail.com>
Committer Checklist (excluded from commit message)