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.

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-15716: KRaft support in EpochDrivenReplicationProtocolAcceptanceTest #15295

Open
wants to merge 23 commits into
base: trunk
Choose a base branch
from

Conversation

highluck
Copy link
Contributor

Committer Checklist (excluded from commit message)

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

@highluck
Copy link
Contributor Author

@stejani-cflt
Please review! thanks :)

@mimaison
Copy link
Member

mimaison commented Feb 2, 2024

Thanks for the PR.
The code does not seem to compile. See the errors in https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-15295/2/pipeline

@highluck
Copy link
Contributor Author

highluck commented Feb 6, 2024

@mimaison
I tried this and that, but I think I need to study a little more.
After finishing this, would it be okay to do follow-up PR work on the shouldFollowLeaderEpochBasicWorkflow function?

@mimaison
Copy link
Member

mimaison commented Feb 6, 2024

The build is still failing with Java 8 and Scala 2.12 so we can't merge this PR as is.
You should be able to reproduce the issue by running: ./gradlew -PscalaVersion=2.12 core:compileTestScala

@highluck
Copy link
Contributor Author

highluck commented Feb 8, 2024

Oh thank you! I'll fix it right away

@highluck
Copy link
Contributor Author

highluck commented Feb 8, 2024

@mimaison
#15342
The test was failing, so I posted a PR first.
If the above PR is merged, I think the test will also succee

@mimaison
Copy link
Member

It seems this breaks all the EpochDrivenReplicationProtocolAcceptanceWithIbp26Test tests, see https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-15295/16/testReport/kafka.server.epoch/

You can run the core tests locally by running ./gradlew core:test.

@pasharik
Copy link
Contributor

Hi, it looks like there is a typo in the ticket number in the title of this pull request. It says KAFKA-15761, but the change is related to KAFKA-15716 - the last two digits are switched, 61 instead of 16

@highluck highluck changed the title KAFKA-15761: KRaft support in EpochDrivenReplicationProtocolAcceptanceTest KAFKA-15716: KRaft support in EpochDrivenReplicationProtocolAcceptanceTest Feb 24, 2024
@highluck
Copy link
Contributor Author

@mimaison @pasharik
thanks for review!
I edited it :)

Copy link
Member

@mimaison mimaison left a 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.

Why are we not updating shouldFollowLeaderEpochBasicWorkflow()? If there's a good reason, please add a comment explaining why, otherwise let's update it like the other tests.

@BeforeEach
override def setUp(testInfo: TestInfo): Unit = {
if (TestInfoUtils.isKRaft(testInfo) && metadataVersion.isLessThan(IBP_3_3_IV0)) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why we need this change? metadataVersion is hard coded to MetadataVersion.latestTesting, how can it be less than IBP_3_3_IV0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh thank you! It seems unnecessary. I'll edit it

@highluck
Copy link
Contributor Author

@mimaison
thanks for review

I felt like I needed to think a little more, so I thought it would be a good idea to work on a follow-up PR.
This is because the test does not work with the broker structure currently in use.
When the current work is finished, I will do a follow-up work!

Copy link
Member

@mimaison mimaison left a 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 still have a few questions.

@ParameterizedTest(name = TestInfoUtils.TestWithParameterizedQuorumName)
@ValueSource(strings = Array("zk", "kraft"))
def shouldNotAllowDivergentLogs(quorum: String): Unit = {
if (quorum == "kraft" && metadataVersion.isLessThan(IBP_3_3_IV0)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this metadata check needed? can you explain it?

@BeforeEach
override def setUp(testInfo: TestInfo): Unit = {
if (TestInfoUtils.isKRaft(testInfo)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we not running setUp() in KRaft mode?

Copy link

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)

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.

@github-actions github-actions bot added the stale Stale PRs label Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale PRs
Projects
None yet
3 participants