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

Reply via email to