[GitHub] [kafka] showuon edited a comment on pull request #10509: KAFKA-12464: enhance constrained sticky Assign algorithm
showuon edited a comment on pull request #10509: URL: https://github.com/apache/kafka/pull/10509#issuecomment-828440406 @ableegoldman , thanks for your great comments! If there's a best reviewer contest, I'd definitely vote for you! :) I've replied your `getUnassignedPartitions` comments. Please take a look. Simply put, I have a further improvement to keep pretty much the same memory usage as before, and still have the performance improvement. I think we should keep this change. :) I'd like to see if you have any thoughts. :) -- 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: us...@infra.apache.org
[GitHub] [kafka] showuon edited a comment on pull request #10509: KAFKA-12464: enhance constrained sticky Assign algorithm
showuon edited a comment on pull request #10509: URL: https://github.com/apache/kafka/pull/10509#issuecomment-816379867 @ableegoldman , please help review this PR. Thank you. -- 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: us...@infra.apache.org
[GitHub] [kafka] showuon edited a comment on pull request #10509: KAFKA-12464: enhance constrained sticky Assign algorithm
showuon edited a comment on pull request #10509: URL: https://github.com/apache/kafka/pull/10509#issuecomment-823030049 jenkins test results after my change: **(avg. 2 sec)** ``` Build / JDK 8 and Scala 2.12 / testLargeAssignmentAndGroupWithUniformSubscription() | 1.8 sec | Passed Build / JDK 11 and Scala 2.13 / testLargeAssignmentAndGroupWithUniformSubscription() | 2.2 sec | Passed Build / JDK 15 and Scala 2.13 / testLargeAssignmentAndGroupWithUniformSubscription() | 2.2 sec | Passed ``` the latest trunk test results: **(avg. 5.4 sec)** ``` Build / JDK 15 and Scala 2.12 / testLargeAssignmentAndGroupWithUniformSubscription() | 6.3 sec | Passed Build / JDK 11 and Scala 2.13 / testLargeAssignmentAndGroupWithUniformSubscription() | 4.4 sec | Passed Build / JDK 11 and Scala 2.12 / testLargeAssignmentAndGroupWithUniformSubscription() | 5.7 sec | Passed Build / JDK 15 and Scala 2.13 / testLargeAssignmentAndGroupWithUniformSubscription() | 6.5 sec | Passed Build / JDK 8 and Scala 2.13 / testLargeAssignmentAndGroupWithUniformSubscription() | 6.3 sec | Passed Build / JDK 8 and Scala 2.12 / testLargeAssignmentAndGroupWithUniformSubscription() | 3.6 sec | Passed ``` BTW, all failed tests are flaky and unrelated: ``` Build / JDK 11 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateListDeleteTopic() Build / JDK 15 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testReplication() Build / JDK 15 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testReplicationWithEmptyPartition() Build / JDK 15 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testOneWayReplicationWithAutoOffsetSync() Build / JDK 15 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTest.testReplicationWithEmptyPartition() Build / JDK 15 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTest.testOneWayReplicationWithAutoOffsetSync() Build / JDK 15 and Scala 2.13 / org.apache.kafka.connect.integration.BlockingConnectorTest.testBlockInSourceTaskStop Build / JDK 15 and Scala 2.13 / org.apache.kafka.connect.integration.BlockingConnectorTest.testBlockInSourceTaskStart Build / JDK 15 and Scala 2.13 / org.apache.kafka.streams.integration.KTableKTableForeignKeyInnerJoinMultiIntegrationTest.shouldInnerJoinMultiPartitionQueryable Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testReplication() Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTest.testReplication() Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTest.testReplicationWithEmptyPartition() Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTest.testOneWayReplicationWithAutoOffsetSync() Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTest.testReplication() Build / JDK 8 and Scala 2.12 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTest.testReplicationWithEmptyPartition() Build / JDK 8 and Scala 2.12 / kafka.server.RaftClusterTest.testCreateClusterAndCreateListDeleteTopic() ``` -- 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: us...@infra.apache.org
[GitHub] [kafka] showuon edited a comment on pull request #10509: KAFKA-12464: enhance constrained sticky Assign algorithm
showuon edited a comment on pull request #10509: URL: https://github.com/apache/kafka/pull/10509#issuecomment-822975764 @ableegoldman (cc. @guozhangwang) After completing the sticky general assignor improvement (https://github.com/apache/kafka/pull/10552), I started to think... why don't I re-use the same techniques to the constrained assignor. In this commit: https://github.com/apache/kafka/pull/10509/commits/cd68d10b5030ecf9c1fd40b518322c0649a33ee4, I adopted the refactor 2 and 4 in https://github.com/apache/kafka/pull/10552 to make the constrained assignor faster! After the PR: the `testLargeAssignmentAndGroupWithUniformSubscription` (1 million partitions) will run from **~2600 ms down to ~1400 ms**, improves **46%** of performance, almost **2x faster**!! Let's what the result is in jenkins trunk build. -- 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: us...@infra.apache.org
[GitHub] [kafka] showuon edited a comment on pull request #10509: KAFKA-12464: enhance constrained sticky Assign algorithm
showuon edited a comment on pull request #10509: URL: https://github.com/apache/kafka/pull/10509#issuecomment-822975764 @ableegoldman (cc. @guozhangwang) After completing the sticky general assignor improvement (https://github.com/apache/kafka/pull/10552), I started to think... why don't I re-use the same techniques to the constrained assignor. In this commit: https://github.com/apache/kafka/pull/10509/commits/cd68d10b5030ecf9c1fd40b518322c0649a33ee4, I adopted the refactor 2 and 4 in https://github.com/apache/kafka/pull/10552 to make the constrained assignor faster! After the PR: the `testLargeAssignmentAndGroupWithUniformSubscription` (1 million partitions) will run from **~2600 ms down to ~1400 ms**, improves **46%** of performance, almost 2x faster!! Let's what the result is in jenkins trunk build. -- 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: us...@infra.apache.org
[GitHub] [kafka] showuon edited a comment on pull request #10509: KAFKA-12464: enhance constrained sticky Assign algorithm
showuon edited a comment on pull request #10509: URL: https://github.com/apache/kafka/pull/10509#issuecomment-822975764 @ableegoldman (cc. @guozhangwang) After completing the sticky general assignor improvement (https://github.com/apache/kafka/pull/10552), I started to think... why don't I re-use the same techniques to the constrained assignor. In this commit: https://github.com/apache/kafka/pull/10509/commits/cd68d10b5030ecf9c1fd40b518322c0649a33ee4, I adopted the refactor 2 and 4 in https://github.com/apache/kafka/pull/10552 to make the constrained assignor faster! After the PR: the `testLargeAssignmentAndGroupWithUniformSubscription` (1 million partitions) will run from **~2600 ms down to ~1400 ms**, improves **46%** of performance!! Let's what the result is in jenkins trunk build. -- 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: us...@infra.apache.org
[GitHub] [kafka] showuon edited a comment on pull request #10509: KAFKA-12464: enhance constrained sticky Assign algorithm
showuon edited a comment on pull request #10509: URL: https://github.com/apache/kafka/pull/10509#issuecomment-816379867 @ableegoldman , please help review this PR. The testLargeAssignmentAndGroupWithUniformSubscription test keeps pretty much the same performance as before, but keep the code simpler and cleaner. Thank you. -- 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: us...@infra.apache.org
