system tests - adding listener overrides#6981
Conversation
|
Let's run this with the branch builder before merging. |
rajinisivaram
left a comment
rajinisivaram
left a comment
There was a problem hiding this comment.
@brianbushree Thanks for the PR. Left a couple of comments. Also, I think it may be neater to define a new class for listener configs that encapsulates client_listener_overrides, interbroker_listener_overridesanduse_separate_interbroker_listener`. We can then add listener names to that class as well in future. It will provide a neater way to propagate these configs to SecurityConfig as well.
|
|
||
| {% if interbroker_listener.name != security_protocol %} | ||
| {% for k, v in interbroker_listener_overrides.iteritems() %} | ||
| listener.name.{{ interbroker_listener.name.lower() }}.{{ security_config.interbroker_sasl_mechanism.lower() }}.{{ k }}={{ v }} |
There was a problem hiding this comment.
Same as before, this assumes listener overrides are only for SASL configs
|
@rajinisivaram I made the requested changes. Could you take another look? Also I am running the tests again now |
rajinisivaram
left a comment
rajinisivaram
left a comment
There was a problem hiding this comment.
@brianbushree Thanks for the update, looks good. One of the comments on the config prefix was not addressed. I have left an explanation of the scenario.
|
out of the two test failures mentioned above, looks like one of them is also failing on |
|
#7000 is meant to fix the upgrade test. @brianbushree are all other tests passing with this PR? |
|
@ijuma all but one. however |
|
http://confluent-kafka-branch-builder-system-test-results.s3-us-west-2.amazonaws.com/2019-06-27--001.1561654600--brianbushree--listener-overrides--ddb024d/report.html |
rajinisivaram
left a comment
rajinisivaram
left a comment
There was a problem hiding this comment.
@brianbushree Thanks for the updates, LGTM
Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>
Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>
Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>
3 participants
what
Now that we have an abstraction for listeners, people should be allowed to pass in certain overrides for their listeners. This allows a user to override both client & interbroker listeners with a dictionary of configs.
why
There may be certain listener configs a user would want to override. Currently, to do so, they would either have to completely override all listener configs OR try to shoehorn an override based on the name of the security-protocol or an internal constant. This makes things a bit cleaner such that we handle the prefixing and simply add on some additional configs.
Committer Checklist (excluded from commit message)