KAFKA-10811: Correct the MirrorConnectorsIntegrationTest to correctly mask the exit procedures#9698
Conversation
|
@C0urante as mentioned offline, here is my fix for the MM2 integration test frequently exiting the builds. |
|
This is definitely an improvement not only to the flaky MM2 test but to the integration testing framework in general. ✅ As a follow-up item, could we track the unclean shutdowns for MM2 we've observed from these tests in a separate ticket? Given the flakiness that they're causing right now, it seems best to temporarily ignore them, but ideally it should be possible to run MM2 without seeing |
|
@C0urante is there a KAFKA issue for those stack traces? If not, please create one. |
|
@rhauch it's not so much about the stack traces in these tests as it is about them presumably appearing when MM2 is run for real, and being caused by unclean worker shutdown. I've written up KAFKA-10812 to track the issue. |
… mask the exit procedures Normally the `EmbeddedConnectCluster` class masks the `Exit` procedures using within the Connect worker. This normally works great when a single instance of the embedded cluster is used. However, the `MirrorConnectorsIntegrationTest` uses two `EmbeddedConnectCluster` instances, and when the first one is stopped it would reset the (static) exit procedures, and any problems during shutdown of the second embedded Connect cluster would cause the worker to shut down the JVM running the tests. Instead, the `MirrorConnectorsIntegrationTest` class should mask the `Exit` procedures and instruct the `EmbeddedConnectClusters` instances (via the existing builder method) to not mask the procedures.
|
Rebased on top of |
| private EmbeddedConnectCluster primary; | ||
| private EmbeddedConnectCluster backup; | ||
|
|
||
| private Exit.Procedure exitProcedure; |
There was a problem hiding this comment.
It seems exitProcedure and haltProcedure can be local variable
There was a problem hiding this comment.
Perhaps, but the log messages for each is different, so IMO it's better to keep them separate.
| shuttingDown = true; | ||
| try { | ||
| try { | ||
| primary.stop(); |
There was a problem hiding this comment.
How about using Utils.closeQuietly to replace this nested try-block?
There was a problem hiding this comment.
EmbeddedConnectCluster is not AutoCloseable, which means we can't use that. Since this PR is to fix broken builds, I'd like to minimize the additional changes, so I'll issue a followup PR.
… mask the exit procedures (#9698) Normally the `EmbeddedConnectCluster` class masks the `Exit` procedures using within the Connect worker. This normally works great when a single instance of the embedded cluster is used. However, the `MirrorConnectorsIntegrationTest` uses two `EmbeddedConnectCluster` instances, and when the first one is stopped it would reset the (static) exit procedures, and any problems during shutdown of the second embedded Connect cluster would cause the worker to shut down the JVM running the tests. Instead, the `MirrorConnectorsIntegrationTest` class should mask the `Exit` procedures and instruct the `EmbeddedConnectClusters` instances (via the existing builder method) to not mask the procedures. Author: Randall Hauch <rhauch@gmail.com> Reviewers: Mickael Maison <mickael.maison@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
… mask the exit procedures (#9698) Normally the `EmbeddedConnectCluster` class masks the `Exit` procedures using within the Connect worker. This normally works great when a single instance of the embedded cluster is used. However, the `MirrorConnectorsIntegrationTest` uses two `EmbeddedConnectCluster` instances, and when the first one is stopped it would reset the (static) exit procedures, and any problems during shutdown of the second embedded Connect cluster would cause the worker to shut down the JVM running the tests. Instead, the `MirrorConnectorsIntegrationTest` class should mask the `Exit` procedures and instruct the `EmbeddedConnectClusters` instances (via the existing builder method) to not mask the procedures. Author: Randall Hauch <rhauch@gmail.com> Reviewers: Mickael Maison <mickael.maison@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
… mask the exit procedures (#9698) Normally the `EmbeddedConnectCluster` class masks the `Exit` procedures using within the Connect worker. This normally works great when a single instance of the embedded cluster is used. However, the `MirrorConnectorsIntegrationTest` uses two `EmbeddedConnectCluster` instances, and when the first one is stopped it would reset the (static) exit procedures, and any problems during shutdown of the second embedded Connect cluster would cause the worker to shut down the JVM running the tests. Instead, the `MirrorConnectorsIntegrationTest` class should mask the `Exit` procedures and instruct the `EmbeddedConnectClusters` instances (via the existing builder method) to not mask the procedures. Author: Randall Hauch <rhauch@gmail.com> Reviewers: Mickael Maison <mickael.maison@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
4 participants
Normally the
EmbeddedConnectClusterclass masks theExitprocedures using within the Connect worker. This normally works great when a single instance of the embedded cluster is used. However, theMirrorConnectorsIntegrationTestuses twoEmbeddedConnectClusterinstances, and when the first one is stopped it would reset the (static) exit procedures, and any problems during shutdown of the second embedded Connect cluster would cause the worker to shut down the JVM running the tests.Instead, the
MirrorConnectorsIntegrationTestclass should mask theExitprocedures and instruct theEmbeddedConnectClustersinstances (via the existing builder method) to not mask the procedures.Ideally this should also be backported to
2.7,2.6, and2.5branches.Committer Checklist (excluded from commit message)