You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/02/12 06:28:37 UTC
[GitHub] [kafka] vvcephei opened a new pull request #10119: Kip 695 revert
vvcephei opened a new pull request #10119:
URL: https://github.com/apache/kafka/pull/10119
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)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on pull request #10119: [DO NOT SQUASH]: revert KIP-695
Posted by GitBox <gi...@apache.org>.
vvcephei commented on pull request #10119:
URL: https://github.com/apache/kafka/pull/10119#issuecomment-778443546
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!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on pull request #10119: Kip 695 revert
Posted by GitBox <gi...@apache.org>.
vvcephei commented on pull request #10119:
URL: https://github.com/apache/kafka/pull/10119#issuecomment-778260311
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on pull request #10119: [DO NOT SQUASH]: revert KIP-695
Posted by GitBox <gi...@apache.org>.
vvcephei commented on pull request #10119:
URL: https://github.com/apache/kafka/pull/10119#issuecomment-778506778
I merged the two commits from this PR after rebasing them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on pull request #10119: Kip 695 revert
Posted by GitBox <gi...@apache.org>.
vvcephei commented on pull request #10119:
URL: https://github.com/apache/kafka/pull/10119#issuecomment-778006301
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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #10119: Kip 695 revert
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #10119:
URL: https://github.com/apache/kafka/pull/10119#issuecomment-778255570
@vvcephei Is this revert included by 2.9? If so, we can introduce the behavior in major release (3.0) straightforward.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on a change in pull request #10119: Kip 695 revert
Posted by GitBox <gi...@apache.org>.
vvcephei commented on a change in pull request #10119:
URL: https://github.com/apache/kafka/pull/10119#discussion_r575413056
##########
File path: streams/src/main/java/org/apache/kafka/streams/StreamsConfig.java
##########
@@ -144,7 +144,6 @@
private static final long EOS_DEFAULT_COMMIT_INTERVAL_MS = 100L;
public static final int DUMMY_THREAD_INDEX = 1;
Review comment:
Thanks, @chia7712 .
Are you pointing out that this doesn't completely revert the commits? This is true. I had to resolve a bunch of conflicts anyway, so I went over the diff and kept most of the "code cleanup" changes that I had done in those commits. That way, code in 2.8 will be as close as possible to trunk, making it easier to cherry-pick fixes in the future.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #10119: Kip 695 revert
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #10119:
URL: https://github.com/apache/kafka/pull/10119#discussion_r575312168
##########
File path: streams/src/main/java/org/apache/kafka/streams/StreamsConfig.java
##########
@@ -144,7 +144,6 @@
private static final long EOS_DEFAULT_COMMIT_INTERVAL_MS = 100L;
public static final int DUMMY_THREAD_INDEX = 1;
Review comment:
There are some refactor code related to KIP-695 (https://github.com/apache/kafka/commit/4d28391480fd8c547a63af119bba67fceb5d2ede#diff-07a8a87d3ee84c3e96918aa131033780cf4bc4d4a12ae376e91c059a9e02d229R142)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei closed pull request #10119: [DO NOT SQUASH]: revert KIP-695
Posted by GitBox <gi...@apache.org>.
vvcephei closed pull request #10119:
URL: https://github.com/apache/kafka/pull/10119
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on pull request #10119: Kip 695 revert
Posted by GitBox <gi...@apache.org>.
vvcephei commented on pull request #10119:
URL: https://github.com/apache/kafka/pull/10119#issuecomment-778370265
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on pull request #10119: [DO NOT SQUASH]: revert KIP-695
Posted by GitBox <gi...@apache.org>.
vvcephei commented on pull request #10119:
URL: https://github.com/apache/kafka/pull/10119#issuecomment-778505369
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org