divijvaidya commented on PR #13284: URL: https://github.com/apache/kafka/pull/13284#issuecomment-1448267997
Thank you for the detailed explanation @C0urante! > so this doesn't really buy us much right now. Oops, my bad! I did not notice that. > this only guarantees that connector configs have been written to the config topic. That is true. We need a more comprehensive check. I was basing this change on the fact that we use similar verification in other tests (see `waitUntilMirrorMakerIsRunning()` in `MirrorConnectorsIntegrationBaseTest`). It's more of a "better than nothing" approach here. > And even if we did tweak this PR to do that kind of blocking, we'd still have to choose a reasonable amount of time to wait for all of these operations to complete. I think that fixing the test by extending the timeout might hide the problems that users may be facing in production as well. Ideally the `MirrorMaker#start()` should be returning a `Future` which could be tracked to check the completion of start. In absence of such a mechanism, today, a user might call `MirrorMaker#start()` and assume that it has started correctly while in the background, connector start up might have failed (just as it did in this test scenario). Hence, I believe, we need a better mechanism to check for successful startup of MirrorMaker. Thoughts? > Do you think it might be worth it to just bump the timeouts of these tests? We could do that AND as a partial safety check block topic creation in this test until connectors have at least been configured (see rejected alternative). Let's reach to a conclusion on the above point first and then we can proceed with this option if that's what we agree to. -- 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
