[GitHub] [kafka] rondagostino commented on pull request #10991: MINOR: system test fix for 3 co-located KRaft controllers

[GitBox <gi...@apache.org>]

rondagostino commented on pull request #10991:
URL: https://github.com/apache/kafka/pull/10991#issuecomment-876031699


   > The idea is that if the controller portion of the combined node is up, it's reasonable to proceed
   
   That's a good idea.  I think the diff above has a problem in that we would emit the message too early for the case where `process.roles=broker`, but I think the idea in general is sound.
   
   Maybe we emit a different message when there is a controller role and that starts, and we emit the same message we emit now after we start the broker (if any).  So it might look like this (Im typing this free-form, so ignore any typos -- it's just to get the point across):
   
   ```     Mx4jLoader.maybeLoad()
        raftManager.startup()
        controller.foreach { c =>
           c.startup()
           info(KafkaBroker.CONTROLLER_STARTED_MESSAGE) // something that doesn't match the regex in the system test
        }
        broker.foreach(_.startup())
        AppInfoParser.registerAppInfo(Server.MetricsPrefix, config.brokerId.toString, metrics, time.milliseconds())
        info(KafkaBroker.STARTED_MESSAGE)
   ```
   
   And then in the system test code, where we check for the log message, we check for the CONTROLLER_STARTED_MESSAGE when the roles include the controller role -- otherwise we check for the standard message.
   
   WDYT?


-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org