You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/12/04 23:51:58 UTC
[GitHub] [kafka] rhauch opened a new pull request #9698: KAFKA-10811: Correct the MirrorConnectorsIntegrationTest to correctly mask the exit procedures
rhauch opened a new pull request #9698:
URL: https://github.com/apache/kafka/pull/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.
Ideally this should also be backported to `2.7`, `2.6`, and `2.5` branches.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on pull request #9698: KAFKA-10811: Correct the MirrorConnectorsIntegrationTest to correctly mask the exit procedures
Posted by GitBox <gi...@apache.org>.
C0urante commented on pull request #9698:
URL: https://github.com/apache/kafka/pull/9698#issuecomment-739083240
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 `ERROR`-level stack traces in logs on shutdown.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rhauch merged pull request #9698: KAFKA-10811: Correct the MirrorConnectorsIntegrationTest to correctly mask the exit procedures
Posted by GitBox <gi...@apache.org>.
rhauch merged pull request #9698:
URL: https://github.com/apache/kafka/pull/9698
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rhauch commented on a change in pull request #9698: KAFKA-10811: Correct the MirrorConnectorsIntegrationTest to correctly mask the exit procedures
Posted by GitBox <gi...@apache.org>.
rhauch commented on a change in pull request #9698:
URL: https://github.com/apache/kafka/pull/9698#discussion_r537599704
##########
File path: connect/mirror/src/test/java/org/apache/kafka/connect/mirror/MirrorConnectorsIntegrationTest.java
##########
@@ -194,20 +224,27 @@ private void waitUntilMirrorMakerIsRunning(EmbeddedConnectCluster connectCluster
@After
public void close() {
- for (String x : primary.connectors()) {
- primary.deleteConnector(x);
- }
- for (String x : backup.connectors()) {
- backup.deleteConnector(x);
- }
- deleteAllTopics(primary.kafka());
- deleteAllTopics(backup.kafka());
- primary.stop();
- backup.stop();
try {
- assertFalse(exited.get());
+ for (String x : primary.connectors()) {
+ primary.deleteConnector(x);
+ }
+ for (String x : backup.connectors()) {
+ backup.deleteConnector(x);
+ }
+ deleteAllTopics(primary.kafka());
+ deleteAllTopics(backup.kafka());
} finally {
- Exit.resetExitProcedure();
+ shuttingDown = true;
+ try {
+ try {
+ primary.stop();
Review 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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rhauch commented on pull request #9698: KAFKA-10811: Correct the MirrorConnectorsIntegrationTest to correctly mask the exit procedures
Posted by GitBox <gi...@apache.org>.
rhauch commented on pull request #9698:
URL: https://github.com/apache/kafka/pull/9698#issuecomment-739084805
@C0urante is there a KAFKA issue for those stack traces? If not, please create one.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #9698: KAFKA-10811: Correct the MirrorConnectorsIntegrationTest to correctly mask the exit procedures
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9698:
URL: https://github.com/apache/kafka/pull/9698#discussion_r537233024
##########
File path: connect/mirror/src/test/java/org/apache/kafka/connect/mirror/MirrorConnectorsIntegrationTest.java
##########
@@ -194,20 +224,27 @@ private void waitUntilMirrorMakerIsRunning(EmbeddedConnectCluster connectCluster
@After
public void close() {
- for (String x : primary.connectors()) {
- primary.deleteConnector(x);
- }
- for (String x : backup.connectors()) {
- backup.deleteConnector(x);
- }
- deleteAllTopics(primary.kafka());
- deleteAllTopics(backup.kafka());
- primary.stop();
- backup.stop();
try {
- assertFalse(exited.get());
+ for (String x : primary.connectors()) {
+ primary.deleteConnector(x);
+ }
+ for (String x : backup.connectors()) {
+ backup.deleteConnector(x);
+ }
+ deleteAllTopics(primary.kafka());
+ deleteAllTopics(backup.kafka());
} finally {
- Exit.resetExitProcedure();
+ shuttingDown = true;
+ try {
+ try {
+ primary.stop();
Review comment:
How about using ```Utils.closeQuietly``` to replace this nested try-block?
##########
File path: connect/mirror/src/test/java/org/apache/kafka/connect/mirror/MirrorConnectorsIntegrationTest.java
##########
@@ -75,14 +74,45 @@
private static final int RECORD_CONSUME_DURATION_MS = 20_000;
private static final int OFFSET_SYNC_DURATION_MS = 30_000;
- private final AtomicBoolean exited = new AtomicBoolean(false);
+ private volatile boolean shuttingDown;
private Map<String, String> mm2Props;
private MirrorMakerConfig mm2Config;
private EmbeddedConnectCluster primary;
private EmbeddedConnectCluster backup;
+ private Exit.Procedure exitProcedure;
Review comment:
It seems ```exitProcedure``` and ```haltProcedure``` can be local variable
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on pull request #9698: KAFKA-10811: Correct the MirrorConnectorsIntegrationTest to correctly mask the exit procedures
Posted by GitBox <gi...@apache.org>.
C0urante commented on pull request #9698:
URL: https://github.com/apache/kafka/pull/9698#issuecomment-739086987
@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](https://issues.apache.org/jira/browse/KAFKA-10812) to track the issue.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rhauch commented on pull request #9698: KAFKA-10811: Correct the MirrorConnectorsIntegrationTest to correctly mask the exit procedures
Posted by GitBox <gi...@apache.org>.
rhauch commented on pull request #9698:
URL: https://github.com/apache/kafka/pull/9698#issuecomment-739081612
@C0urante as mentioned offline, here is my fix for the MM2 integration test frequently exiting the builds.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rhauch commented on pull request #9698: KAFKA-10811: Correct the MirrorConnectorsIntegrationTest to correctly mask the exit procedures
Posted by GitBox <gi...@apache.org>.
rhauch commented on pull request #9698:
URL: https://github.com/apache/kafka/pull/9698#issuecomment-739529751
Rebased on top of `trunk` to incorporate an upstream fix for the StreamsThreadTest compile error.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rhauch commented on a change in pull request #9698: KAFKA-10811: Correct the MirrorConnectorsIntegrationTest to correctly mask the exit procedures
Posted by GitBox <gi...@apache.org>.
rhauch commented on a change in pull request #9698:
URL: https://github.com/apache/kafka/pull/9698#discussion_r537598810
##########
File path: connect/mirror/src/test/java/org/apache/kafka/connect/mirror/MirrorConnectorsIntegrationTest.java
##########
@@ -75,14 +74,45 @@
private static final int RECORD_CONSUME_DURATION_MS = 20_000;
private static final int OFFSET_SYNC_DURATION_MS = 30_000;
- private final AtomicBoolean exited = new AtomicBoolean(false);
+ private volatile boolean shuttingDown;
private Map<String, String> mm2Props;
private MirrorMakerConfig mm2Config;
private EmbeddedConnectCluster primary;
private EmbeddedConnectCluster backup;
+ private Exit.Procedure exitProcedure;
Review comment:
Perhaps, but the log messages for each is different, so IMO it's better to keep them separate.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org