KAFKA-14718: Fix flaky DedicatedMirrorIntegrationTest#13284
Conversation
|
@C0urante please take a look when you get a chance. |
C0urante
left a comment
C0urante
left a comment
There was a problem hiding this comment.
Thanks Divij! I think we're onto something here, but this PR doesn't quite accomplish what it sets out to.
Although we're changing the semantics of the startLatch field here, we don't actually synchronously block on it (i.e., via CountDownLatch::await) anywhere in the code base except in the shutdown hook set up by MirrorMaker, so this doesn't really buy us much right now.
It's also worth noting that, even if we do successfully block on the completion of the callbacks passed to Herder::putConnectorConfig, this only guarantees that connector configs have been written to the config topic. It does not guarantee that the ensuing rebalance (to allocate the new connector) has completed, that task configs have been generated, that the second ensuing rebalance (to allocate tasks) has completed, or that tasks have been successfully started. So, although the approach that's pursued here does have some merit, it doesn't necessarily prevent the test failures we've been seeing from taking place, especially in slow environments like our Jenkins nodes. 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.
Do you think it might be worth it to just bump the timeouts of these tests? We might go from 30 seconds to, say, 2 minutes. This would make things worse for local development if the changes that someone is testing actually cause the tests to fail, but at this point it seems like the cost of CI flakiness might outweigh that; would be interested in your thoughts.
|
Thank you for the detailed explanation @C0urante!
Oops, my bad! I did not notice that.
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
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
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. |
I don't think that bumping the timeouts in these tests is likely to hide issues that may surface in production; rather, I'm only worried that they'll make local testing and development more cumbersome. It's also worth noting that no matter how precise we make the wait-for-startup logic, we'll still probably end up effectively bumping timeouts in this PR (if we add a 30-second wait for MM2 startup, then that turns the existing 30-second timeout for initial topic replication into 60 seconds).
Agreed 👍 IMO MM2 observability in general can be improved. Right now you're basically limited to JMX, other custom metrics reporters, and logs. There's no public API for viewing cluster-wide health info (the closest thing is reading directly from the status topic, but that's not public API and we shouldn't count on users doing that or recommend it). If there's an issue starting one of the connectors, instead of failing startup of the MM2 node, we log a message and move on. I can't imagine this is very easy for users to work with, though maybe I'm underestimating the utility of JMX and logs here.
This is less concerning because the I'm wondering if, instead of a A brief sketch: public class MirrorMaker {
public ConnectorStateInfo connectorStatus(String source, String target, String connector) {
SourceAndTarget sourceAndTarget = new SourceAndTarget(source, target);
checkHerder(sourceAndTarget);
return herders.get(new SourceAndTarget(source, target)).connectorStatus(connector);
}
}We could then leverage this API in the |
It's not technically part of public API but it has a I agree to the creation of Meanwhile, I will add |
divijvaidya
changed the title
divijvaidya
changed the title
|
@C0urante ready for your review. I have increased the timeout to 2 min. and also added the wait for mirror maker to get ready. In a separate JIRA we should talk about how we can make the startup synchronous. I have created one such JIRA at https://issues.apache.org/jira/browse/KAFKA-14773 |
|
The test failures seems unrelated (and |
C0urante
left a comment
C0urante
left a comment
There was a problem hiding this comment.
Thanks @divijvaidya, this is closer to what I had in mind.
I'd like to clarify again that the semantics of MirrorMaker::start do not matter; instead, the user-facing behavior of the MM2 startup script is what matters here. The invocation of that script encapsulates the entire lifetime of MM2, including the creation of connectors and waiting for the process to shut down. It is not necessary to make that method synchronous since there are no problems with the user-facing behavior right now, at least in that regard. And sure, we may choose to improve MM2 observability in general, but IMO that has very little to do with the start method as more than a minor implementation detail.
With regards to this suggestion:
we should block the completion of
main()until all connectors are online.
This already happens, unless the process is prematurely terminated. Feel free to test this scenario out yourself by running MM2 in dedicated mode and taking a thread dump at any point in the lifetime of the program.
07a0b3a to
e00dae4
Compare
|
Hey @C0urante I am not very much familiar with MM code and connect framework. If you have a suggestion to fix it, I would be happy to do so, otherwise, I will probably need some time to dig into this further. |
|
@divijvaidya No worries about the delay--looks like we're both guilty on that front 😅 I took at look at this and the issue is that the Correct me if I'm wrong: the idea here is to add granularity to test failure messages so that we can differentiate between MM2 startup issues (caused by lagginess or connector/task failures) and issues with the actual replication logic performed by MM2. If that's the case, do you think it might make sense to do the following?
|
|
@C0urante thank you for the suggestions. The new code should be close to what you suggested. The test passes locally for me now. Please feel free to review when you get a chance. |
Test failures are unrelated. @C0urante pinging for review when you get a chance. |
…e connector tasks (#13819) Discovered while researching KAFKA-14718 Currently, we perform a check during zombie fencing that causes the round of zombie fencing to fail when a rebalance is pending (i.e., when we've detected from a background poll of the config topic that a new connector has been created, that an existing connector has been deleted, or that a new set of connector tasks has been generated). It's possible but not especially likely that this check causes issues when running vanilla Kafka Connect. Even when it does, it's easy enough to restart failed tasks via the REST API. However, when running MirrorMaker 2 in dedicated mode, this check is more likely to cause issues as we write three connector configs to the config topic in rapid succession on startup. And in that mode, there is no API to restart failed tasks aside from restarting the worker that they are hosted on. In either case, this check can lead to test flakiness in integration tests for MirrorMaker 2 both in dedicated mode and when deployed onto a vanilla Kafka Connect cluster. This check is not actually necessary, and we can safely remove it. Copied from Jira: >If the worker that we forward the zombie fencing request to is a zombie leader (i.e., a worker that believes it is the leader but in reality is not), it will fail to finish the round of zombie fencing because it won't be able to write to the config topic with a transactional producer. >If the connector has just been deleted, we'll still fail the request since we force a read-to-end of the config topic and refresh our snapshot of its contents before checking to see if the connector exists. >And regardless, the worker that owns the task will still do a read-to-end of the config topic and verify that (1) no new task configs have been generated for the connector and (2) the worker is still assigned the connector, before allowing the task to process any data. In addition, while waiting on a fix for KAFKA-14718 that adds more granularity for diagnosing failures in the DedicatedMirrorIntegrationTest suite (#13284), some of the timeouts in that test are bumped to work better on our CI infrastructure. Reviewers: Mickael Maison <mickael.maison@gmail.com>, Yash Mayya <yash.mayya@gmail.com>, Viktor Somogyi-Vass <viktorsomogyi@gmail.com>
C0urante
left a comment
C0urante
left a comment
There was a problem hiding this comment.
Thanks Divij! One last round of thoughts and then this should be good to go.
|
I had to rebase with trunk to resolve merge conflicts. Changes in latest revision. 1\ Moved This test passes locally. @C0urante ready for review. |
|
Thank you for your patience on this one @C0urante. Appreciate it! |
2 participants
Motivation
The test suite
DedicatedMirrorIntegrationTestfails in a flaky manner with the following error:Root cause of the failure is available in stdout:
The test tries to assert topic replication on the second cluster but the mirror maker nodes themselves are unavailable. They are unavailable because the connectors fail to configure. Connectors fail to configure because the worker is already shutting down by the time they are ready to be configured.
Change
This highlights a gap in MirrorMaker startup sequence where it does not wait for connectors to be configured by the herders before declaring itself as "started". This leads to a situation where MirrorMaker claims it has started but the herders are still in the process of configuring the connectors. During this time, mirror maker will not perform any replication.
With this change, we add a dependency on connector configuration for the MirrorMaker to declare itself as started.
Rejected alternative
An alternative approach is to modify the test to wait for connectors to get configured on all 3 MM nodes before starting the actual test i.e. before making a call to create topic. But this alternative is artificially hiding the problems which users of MirrorMaker may also face.