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-16272: Adding new coordinator related changes for connect_distributed.py #15594
Conversation
There was a problem hiding this 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 @vamossagar12!
This looks good to me. In terms of running the tests, does the figure of 288 tests include the test permutations that aren't affected by this change? It is possible to create a test suite file that lists out the parameterized permutations to test.
Thanks!
There was a problem hiding this 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 @vamossagar12!
@lucasbru—can you take a look at this system test change? Thanks! |
Thanks @kirktrue . I ran a single test by passing a parameter
The test suite file approach doesn't seem to be working when passing json parameters as suggested here:
as the tests aren't getting identified. |
@vamossagar12 did the test you ran pass? Here is an example how I run parameterized tests using a test suite file:
The change looks fine to me. If you want to be sure that the test set up works, you may want to run the parameter combinations and post the results here. However, if you have tested one parameter combination successfully, and you are confident that the general test setup is working, I am fine with merging it like this (please confirm). |
@lucasbru , that test did pass. However, let me try again with the snippet you shared above and see if it works. Let me get back to you. |
051d35c
to
efab63b
Compare
hey @lucasbru , I ran the following test suite
and here are the results:
The failing test The other thing I noticed is that the kafka consumers created in the workers don't adhere to the above |
Removing unnecessary file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Summary of the changes:
consumer.override.
override config when the new group coordinator is enabled.Note about testing: There are 288 tests that need to be run and running on my local takes a lot of time. I will try to post the test results once I have a full run.