Fix describe config for multi-broker clusters #1869
Conversation
There was a problem hiding this comment.
Tests should be based on pytest, not unittest so that #1196 is not blocked.
|
nudge @jlandersen |
apologies for the late reply, been a bit busy + traveling as well. |
Currently all describe config requests are sent to "least loaded node". Requests for broker configs must, however, be sent to the specific broker, otherwise an error is returned. Only topic requests can be handled by any node. This changes the logic to send all describe config requests to the specific broker.
cfa2424 to
cb9e664
Compare
|
Got some time to walk over it before I expected :)
One remaining comment is the handling of ValueError (see at the original comment) |
There was a problem hiding this comment.
Almost there... need to drop the simple_client fixture as that's going away in #1196... hopefully we can get that landed very soon once we are sure that 1.4.7 doesn't need any urgent patch releases.
test/test_admin_integration.py
Outdated
|
|
||
| @pytest.mark.skipif(env_kafka_version() < (0, 11), reason="Describe config features require broker >=0.11") | ||
| def test_describe_configs_topic_resource_returns_configs(simple_client, kafka_admin_client): | ||
| topic = simple_client.topics[0] |
There was a problem hiding this comment.
I think this is available via kafka_admin_client._client.topics()?
There was a problem hiding this comment.
Close :) but by default it doesen't seem like the kafka admin fixture creates topics for tests, so added "topic" as parameter instead.
Also removes dependency on simple_client in tests
|
@jeffwidman changes should be in - let me know of any other things to change! Update: cancel that - some tests seem to have some trouble now with the topic fixture. Is this the right way to setup topics for tests? Another update: seems like the order of fixture arguments matter :-) switched around |
Currently all describe config requests are sent to "least loaded node". Requests for broker configs must, however, be sent to the specific broker, otherwise an error is returned. Only topic requests can be handled by any node.
This changes the logic to send all describe config requests to the specific broker.
Same should be done for alter configs (there is already a comment stating this should be done - I'll be happy to send a PR for this soon'ish).
This change is