cmccabe commented on PR #15986: URL: https://github.com/apache/kafka/pull/15986#issuecomment-2138338743
Thanks for the PR, @jsancio . A few meta-comments: - I'm not sure I see the benefit to changing the `toString` functions to use `String.format`. It seems more brittle than the simple string concatenation approach. Unless you want to print something in a specific way, like `String.format("%02d", myInt)`. But that isn't the case here. - Changing the SharedServer constructor is a huge pain and generates a lot of churn. Since the thing you're passing comes from the static configuration anyway, let's not do that. - I think we should try to avoid doing too much validation in `KafkaConfig`. Things like hostnames should be resolved when we actually need them. It would be silly for one unresolvable hostname to make configuration validation fail, and hence fail the whole kcontroller startup process. - I don't think we want to change all of the tests to use `controller.quorum.bootstrap.servers`. We still need to support the old configuration. Let's make it so that tests using IBP_3_8_IV0 or newer use the new thing, and tests using an older MV use the old configuration. That way we will have good coverage. -- 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: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org