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

[DO NOT SQUASH]: revert KIP-695 #10119

Closed
wants to merge 2 commits into from
Closed

Conversation

vvcephei
Copy link
Contributor

Reverts both commits in KIP-695, which is being postponed until after 2.8

Committer Checklist (excluded from commit message)

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

@vvcephei
Copy link
Contributor Author

Hey @guozhangwang , unfortunately, the revert wasn't clean. I kept the two commits separate (and intend to merge them separately).

I'm pretty sure I resolved the reversion correctly. Can you make a quick pass to double-check I didn't break something?

@chia7712
Copy link
Contributor

@vvcephei Is this revert included by 2.9? If so, we can introduce the behavior in major release (3.0) straightforward.

@vvcephei
Copy link
Contributor Author

Thanks, @chia7712 !

With all the recent feedback and no clear "best" option, I've gone back to the drawing board this week, and I think I've figured out a better way to do it. I was going to prepare a PR for trunk and then send a reply to the mailing list.

I decided to just revert the feature from 2.8 instead of changing the design and putting in a fresh implementation so long after feature freeze. I should have reverted it right away, since it's blocking the system tests; I was just hoping to rescue it for 2.8.

@vvcephei
Copy link
Contributor Author

Hmm. The java 15 build passed, java 11 failed in the core SSL test (flaky), and java 8 "exited with a non-zero exit status" twice in a row. The Jenkins logs are a little hard to read, but it looks like it failed in the core integration tests, which pass for me using java 8.

I ran it locally on java 8, though, and it passes for me.

I'm running the Jenkins build again, and also running the full integration test suite in java 8 on my machine. If it continues to look like a flaky failure, I'll just go ahead and merge it.

Copy link
Contributor

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

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

@vvcephei Could you please revise title to make it more clear?

@vvcephei vvcephei changed the title Kip 695 revert [DO NOT SQUASH]: revert KIP-695 Feb 12, 2021
@vvcephei
Copy link
Contributor Author

Sure thing, @chia7712 . For the record, I don't plan to merge this PR normally.

I wanted to maintain a 1:1 relationship between the initial commits and their reversions, but just included them both in one PR for ease of review and testing.

Once the tests finish, I'll merge the commits individually and then close this PR.

Thanks for the review!

@vvcephei
Copy link
Contributor Author

Ok, at least all the jvms completed the test suite this time, but I still see a set of "usual suspects" flaky tests:

    Build / JDK 8 / kafka.api.PlaintextAdminIntegrationTest.testDescribeLogDirs()
    Build / JDK 11 / org.apache.kafka.connect.integration.RebalanceSourceConnectorsIntegrationTest.testMultipleWorkersRejoining
    Build / JDK 15 / org.apache.kafka.connect.integration.BlockingConnectorTest.testBlockInSinkTaskStart
    Build / JDK 15 / org.apache.kafka.streams.integration.KTableKTableForeignKeyInnerJoinMultiIntegrationTest.shouldInnerJoinMultiPartitionQueryable

I have checked and see these same tests failing on 2.8, so I will proceed to merge and hopefully getting the system tests unblocked.

vvcephei pushed a commit that referenced this pull request Feb 12, 2021
This reverts commit 4d28391.

Closes #10119

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
vvcephei pushed a commit that referenced this pull request Feb 12, 2021
This reverts commit fdcf8fb.

Closes #10119

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
@vvcephei vvcephei closed this Feb 12, 2021
@vvcephei
Copy link
Contributor Author

I merged the two commits from this PR after rebasing them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants