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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
