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 2022/05/21 13:15:15 UTC
[GitHub] [kafka] C0urante opened a new pull request, #12191: KAFKA-12657: Increase timeouts in Connect integration tests
C0urante opened a new pull request, #12191:
URL: https://github.com/apache/kafka/pull/12191
[Jira](https://github.com/C0urante/kafka/pull/new/kafka-12657)
As an initial step to address the notoriously flaky `BlockingConnectorTest` test suite, we can try increasing test timeouts.
This approach may not be sufficient, and even if it is, it's still suboptimal. Although it may address flakiness on Jenkins, it will make genuine failures harder to detect when testing local changes. Additionally, if the workload on Jenkins continues to increase, we'll probably have to bump these timeouts in the future again at some point.
Potential next steps, for this PR and beyond:
1. Stop leaking threads that block during test runs
2. Test repeatedly on Jenkins, ideally at least 50 times
3. Gather information on the number of CPU cores available to each Jenkins node and the distribution of how many threads are allocated over a given time period (maybe a day?); this is especially relevant since local testing indicates that these tests all do much better when parallelism is reduced, which shouldn't be too surprising considering that each Connect integration test spins up separate threads for at least one Zookeeper node, one Kafka broker, one Connect worker, and usually at least one connector and one task.
I'd like to test these changes as a first step before investigating any of the above (except maybe item 1, which should be fairly straightforward). To trigger new runs I plan on pushing empty commits or, if those do not trigger new Jenkins runs, dummy commits. If this is objectionable let me know and hopefully we can find a suitable alternative.
### 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on pull request #12191: KAFKA-12657: Increase timeouts in Connect integration tests
Posted by GitBox <gi...@apache.org>.
C0urante commented on PR #12191:
URL: https://github.com/apache/kafka/pull/12191#issuecomment-1133779567
Looking good so far. All Connect integration tests passed in every build that was able to run them at all.
Going to try to trigger another round of tests now by pushing an empty commit; if that doesn't work, may try a few other things.
--
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
[GitHub] [kafka] C0urante commented on a diff in pull request #12191: KAFKA-12657: Increase timeouts in Connect integration tests
Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12191:
URL: https://github.com/apache/kafka/pull/12191#discussion_r880460302
##########
connect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedConnectClusterAssertions.java:
##########
@@ -44,9 +44,9 @@
public class EmbeddedConnectClusterAssertions {
private static final Logger log = LoggerFactory.getLogger(EmbeddedConnectClusterAssertions.class);
- public static final long WORKER_SETUP_DURATION_MS = TimeUnit.SECONDS.toMillis(60);
+ public static final long WORKER_SETUP_DURATION_MS = TimeUnit.MINUTES.toMillis(5);
Review Comment:
I'm worried that with incremental increases, even if things are sufficient for now, they may become insufficient again in the near future. Given how infrequently changes that affect worker startup are (which might lead to this timeout elapsing before a true test failure is surfaced) and the number of moving parts that have to get initialized for an embedded Connect cluster to get off the ground, it seems worth the tradeoff to make the timeout high here.
--
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
[GitHub] [kafka] C0urante commented on pull request #12191: KAFKA-12657: Increase timeouts in Connect integration tests
Posted by GitBox <gi...@apache.org>.
C0urante commented on PR #12191:
URL: https://github.com/apache/kafka/pull/12191#issuecomment-1133669242
Going to keep this PR as-is until we can get more data on whether increasing the timeouts is sufficient for Jenkins.
--
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
[GitHub] [kafka] cadonna commented on a diff in pull request #12191: KAFKA-12657: Increase timeouts in Connect integration tests
Posted by GitBox <gi...@apache.org>.
cadonna commented on code in PR #12191:
URL: https://github.com/apache/kafka/pull/12191#discussion_r880435973
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/BlockingConnectorTest.java:
##########
@@ -78,8 +79,8 @@ public class BlockingConnectorTest {
private static final String NORMAL_CONNECTOR_NAME = "normal-connector";
private static final String TEST_TOPIC = "normal-topic";
private static final int NUM_RECORDS_PRODUCED = 100;
- private static final long CONNECT_WORKER_STARTUP_TIMEOUT = TimeUnit.SECONDS.toMillis(60);
- private static final long RECORD_TRANSFER_DURATION_MS = TimeUnit.SECONDS.toMillis(30);
+ private static final long CONNECTOR_BLOCK_TIMEOUT_MS = TimeUnit.MINUTES.toMillis(2);
+ private static final long RECORD_TRANSFER_TIMEOUT_MS = TimeUnit.MINUTES.toMillis(2);
Review Comment:
Have you tried just doubling the original 30 seconds to 1 minute?
##########
connect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedConnectClusterAssertions.java:
##########
@@ -44,9 +44,9 @@
public class EmbeddedConnectClusterAssertions {
private static final Logger log = LoggerFactory.getLogger(EmbeddedConnectClusterAssertions.class);
- public static final long WORKER_SETUP_DURATION_MS = TimeUnit.SECONDS.toMillis(60);
+ public static final long WORKER_SETUP_DURATION_MS = TimeUnit.MINUTES.toMillis(5);
Review Comment:
This seems quite an increase. Have you tried with 2 minutes?
--
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
[GitHub] [kafka] Kvicii commented on pull request #12191: KAFKA-12657: Increase timeouts in Connect integration tests
Posted by GitBox <gi...@apache.org>.
Kvicii commented on PR #12191:
URL: https://github.com/apache/kafka/pull/12191#issuecomment-1133644673
Sure, but I think itโs a normal PR๐
> On May 21, 2022, at 22:23, Chris Egerton ***@***.***> wrote:
>
>
--
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
[GitHub] [kafka] C0urante commented on pull request #12191: KAFKA-12657: Increase timeouts in Connect integration tests
Posted by GitBox <gi...@apache.org>.
C0urante commented on PR #12191:
URL: https://github.com/apache/kafka/pull/12191#issuecomment-1143540485
@cadonna it is not related to the changes from this PR. I suspect it's caused by the artificial reduction in the REST request timeout that we use in the blocking connector tests in order to reduce test time. Unfortunately, that behavior leaks to other tests being run concurrently, which can cause spurious timeout failures.
I put together https://github.com/C0urante/kafka/pull/2 to address that issue. Up to you if you'd like me to merge that change into this PR so that we can test it in conjunction with the timeout increases here, or if you'd prefer to leave this one as-is and review that as a follow-up item.
--
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
[GitHub] [kafka] C0urante commented on pull request #12191: KAFKA-12657: Increase timeouts in Connect integration tests
Posted by GitBox <gi...@apache.org>.
C0urante commented on PR #12191:
URL: https://github.com/apache/kafka/pull/12191#issuecomment-1133669167
Drafted an approach for item 2 (that also addresses the major issue with item 3, which is that artificially reducing the REST request timeout impacts unrelated Connect integration tests that happen to be running at the time) with https://github.com/C0urante/kafka/pull/2
--
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
[GitHub] [kafka] cadonna merged pull request #12191: KAFKA-12657: Increase timeouts in Connect integration tests
Posted by GitBox <gi...@apache.org>.
cadonna merged PR #12191:
URL: https://github.com/apache/kafka/pull/12191
--
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
[GitHub] [kafka] cadonna commented on pull request #12191: KAFKA-12657: Increase timeouts in Connect integration tests
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12191:
URL: https://github.com/apache/kafka/pull/12191#issuecomment-1143548913
I think it is fine to merge this PR as is. I was just not sure if the failure was an indication that this PR has an 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] C0urante commented on pull request #12191: KAFKA-12657: Increase timeouts in Connect integration tests
Posted by GitBox <gi...@apache.org>.
C0urante commented on PR #12191:
URL: https://github.com/apache/kafka/pull/12191#issuecomment-1133668291
Drafted an approach for item 1 with https://github.com/C0urante/kafka/pull/1
--
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
[GitHub] [kafka] C0urante commented on pull request #12191: KAFKA-12657: Increase timeouts in Connect integration tests
Posted by GitBox <gi...@apache.org>.
C0urante commented on PR #12191:
URL: https://github.com/apache/kafka/pull/12191#issuecomment-1133642820
๐ thanks for the LGTM but I think we may want to wait for some test results first!
--
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
[GitHub] [kafka] C0urante commented on pull request #12191: KAFKA-12657: Increase timeouts in Connect integration tests
Posted by GitBox <gi...@apache.org>.
C0urante commented on PR #12191:
URL: https://github.com/apache/kafka/pull/12191#issuecomment-1134157793
Last run was also green, Connect-wise. Triggering another.
--
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
[GitHub] [kafka] C0urante commented on pull request #12191: KAFKA-12657: Increase timeouts in Connect integration tests
Posted by GitBox <gi...@apache.org>.
C0urante commented on PR #12191:
URL: https://github.com/apache/kafka/pull/12191#issuecomment-1144196063
Sounds good. I suspect the changes from https://github.com/C0urante/kafka/pull/2 might help with some of the flakiness in the `RebalanceSourceConnectorsIntegrationTest`; can file a follow-up if/when this gets merged.
--
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
[GitHub] [kafka] C0urante commented on pull request #12191: KAFKA-12657: Increase timeouts in Connect integration tests
Posted by GitBox <gi...@apache.org>.
C0urante commented on PR #12191:
URL: https://github.com/apache/kafka/pull/12191#issuecomment-1135346345
@cadonna It looks like the timeout increase yields some benefit, at least for the PR builder. Do you have any thoughts on other ways to test this before merging or do you think it's good enough as-is?
--
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
[GitHub] [kafka] C0urante commented on a diff in pull request #12191: KAFKA-12657: Increase timeouts in Connect integration tests
Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12191:
URL: https://github.com/apache/kafka/pull/12191#discussion_r880460797
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/BlockingConnectorTest.java:
##########
@@ -78,8 +79,8 @@ public class BlockingConnectorTest {
private static final String NORMAL_CONNECTOR_NAME = "normal-connector";
private static final String TEST_TOPIC = "normal-topic";
private static final int NUM_RECORDS_PRODUCED = 100;
- private static final long CONNECT_WORKER_STARTUP_TIMEOUT = TimeUnit.SECONDS.toMillis(60);
- private static final long RECORD_TRANSFER_DURATION_MS = TimeUnit.SECONDS.toMillis(30);
+ private static final long CONNECTOR_BLOCK_TIMEOUT_MS = TimeUnit.MINUTES.toMillis(2);
+ private static final long RECORD_TRANSFER_TIMEOUT_MS = TimeUnit.MINUTES.toMillis(2);
Review Comment:
This would probably be safe. I'll push a change and we can give it a try.
--
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
[GitHub] [kafka] cadonna commented on pull request #12191: KAFKA-12657: Increase timeouts in Connect integration tests
Posted by GitBox <gi...@apache.org>.
cadonna commented on PR #12191:
URL: https://github.com/apache/kafka/pull/12191#issuecomment-1143511920
@C0urante I ran the builds a couple of times in run into the following test failure:
https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-12191/7/testReport/junit/org.apache.kafka.connect.integration/RebalanceSourceConnectorsIntegrationTest/Build___JDK_11_and_Scala_2_13___testRemovingWorker/
Is this failure related to this 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.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org