C0urante commented on PR #14630: URL: https://github.com/apache/kafka/pull/14630#issuecomment-1785353764
@kumarpritam863 I understand that the code changes are small, and I'm glad to hear that pre-prod testing indicates that this commit has the intended effect. However, I still think that unit testing coverage is warranted--we want to make sure not just that this change works now, but that we don't accidentally regress in the future. I think it should be possible to use the [MockConsumer class](https://github.com/apache/kafka/blob/9dbee599f13997effd8f7e278fd7256b850c8813/clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java) (possibly with some small tweaks to is [rebalance method](https://github.com/apache/kafka/blob/9dbee599f13997effd8f7e278fd7256b850c8813/clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java#L106-L116) to make the ordering of steps more closely match the ordering in the `KafkaConsumer` class when rebalances take place) to write a new unit test in the [WorkerSinkTaskTest suite](https://github.com/apache/kafka/blob/9dbee599f13997effd8f7e278fd7256b850c8813/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerSinkTask.java) that verifies that the partition-count metric is updated appropriately. You may also notice that existing unit tests are failing now (see the CI results, or feel free to test this PR locally with `./gradlew :connect:runtime:unitTest`). Can you please fix the failing tests and make sure that the build works (at least locally) before requesting review again? Finally, can you update the title of the PR to describe what changes are actually made? You can see examples of good PR titles in the list of [recently-merged commits to trunk](https://github.com/apache/kafka/commits/trunk). Thanks! -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
