Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KAFKA-16813: Add global timeout for @ClusterTemplate, @ClusterTest and @ClusterTests #16957

Merged
merged 6 commits into from
Sep 20, 2024
Merged

KAFKA-16813: Add global timeout for @ClusterTemplate, @ClusterTest and @ClusterTests #16957

merged 6 commits into from
Sep 20, 2024

Conversation

m1a2st
Copy link
Contributor

@m1a2st m1a2st commented Aug 22, 2024 •
edited
Loading

Jira: https://issues.apache.org/jira/browse/KAFKA-16813

add ClusterTest and ClusterTests timout for 30 seconds.

This is testing for GitHub Action CI.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@chia7712 chia7712 mentioned this pull request Aug 22, 2024
Merged
3 tasks
@chia7712
Copy link
Member

@m1a2st please rebase code and fix conflicts

@chia7712 chia7712 mentioned this pull request Aug 22, 2024
Closed
3 tasks
@m1a2st m1a2st force-pushed the gh-KAFKA-17260 branch 2 times, most recently from f904c3a to 87526db Compare August 22, 2024 08:57
@mumrah
Copy link
Member

mumrah commented Aug 23, 2024

@m1a2st can you pull in the latest trunk changes? I have added code to report the JUnit failures in the Github Actions workflows. I want to do a few runs with this change to ensure we haven't introduced a bunch of timeout failures

@chia7712
Copy link
Member

@m1a2st could you please add timeout to @ClusterTemplate?

@m1a2st
Copy link
Contributor Author

m1a2st commented Aug 24, 2024 •
edited
Loading

@m1a2st can you pull in the latest trunk changes? I have added code to report the JUnit failures in the Github Actions workflows. I want to do a few runs with this change to ensure we haven't introduced a bunch of timeout failures

I rebased it.

@m1a2st could you please add timeout to @ClusterTemplate?

I think I should add a constant in TestUtils for 30 second, It will be more easy to controll this parameter. WDYT?

@chia7712
Copy link
Member

I think I should add a constant in TestUtils for 30 second, It will be more easy to controll this parameter. WDYT?

there are only 3 annotations needing the @Timeout(30), so it should be fine to add the timeout annotation individually.

@chia7712
Copy link
Member 

| ❌ | LagFetchIntegrationTest | shouldFetchLagsDuringRebalancingWithNoOptimization() | java.lang.AssertionError: 
Expected: <0L>
     but: was <5L> | 63.29s |

I have filed https://issues.apache.org/jira/browse/KAFKA-17418 to fix the markdown

@chia7712
Copy link
Member

chia7712 commented Sep 2, 2024

@m1a2st Could you please list the timeout tests? Maybe we can file PR to speedup them

@m1a2st
Copy link
Contributor Author

m1a2st commented Sep 3, 2024

I will check these tests which timeout 30 second

  • ResetConsumerGroupOffsetTest.testResetOffsetsToEarliestOnTopicsAndPartitions
  • ResetConsumerGroupOffsetTest.testResetOffsetsAllTopicsAllGroups
  • DescribeConsumerGroupTest.testDescribeWithMultiPartitionTopicAndMultipleConsumers
  • DescribeConsumerGroupTest.testDescribeWithConsumersWithoutAssignedPartitions
  • ZkMigrationIntegrationTest.testMigrateTopicDeletions
  • ClientTelemetryTest.testClientInstanceId

This test is timeout for 120s

  • KafkaAdminClientTest.testClientSideTimeoutAfterFailureToReceiveResponse

This is flaky

@chia7712
Copy link
Member

chia7712 commented Sep 3, 2024

testClientSideTimeoutAfterFailureToReceiveResponse is known flaky (https://issues.apache.org/jira/browse/KAFKA-15916)

ZkMigrationIntegrationTest is a bit complicated, so it should have different timeout. The other IT needs to be improved to reduce the elapsed time. Could you please file sub-tasks (jira) for each test class?

@mumrah
Copy link
Member

mumrah commented Sep 3, 2024

@chia7712 I have a separate PR for ZkMigrationIntegrationTest #17004. This test definitely needs to set its own timeouts as it is rather complicated. In retrospect, some parts of this tests probably should have been system tests.

@m1a2st
Copy link
Contributor Author

m1a2st commented Sep 3, 2024

I open these jira tasks to trace these timeout tests
https://issues.apache.org/jira/browse/KAFKA-17471
https://issues.apache.org/jira/browse/KAFKA-17472
https://issues.apache.org/jira/browse/KAFKA-17473

@chia7712
Copy link
Member

chia7712 commented Sep 4, 2024

@m1a2st Could you please rebase code to trigger QA again?

@chia7712
Copy link
Member

chia7712 commented Sep 9, 2024

@m1a2st Could you please increase the timeout from 30 secs to 60 secs? It seems to me 30 secs is too small. Also, please merge trunk to run CI again

chia7712
chia7712 reviewed Sep 13, 2024
View reviewed changes
core/src/test/java/kafka/test/annotation/ClusterTest.java Outdated
@@ -34,6 +35,7 @@
@Target({METHOD})
@Retention(RUNTIME)
@TestTemplate
@Timeout(30)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m1a2st please change it to 30 second :)

@chia7712
Copy link
Member

#17117 is merged now, so @m1a2st could you please rebase code?

@chia7712
Copy link
Member

@m1a2st Could you please check the failed tests? It seems no cluster tests have timeout error, so maybe we can keep this "60 seconds" WDYT?

@m1a2st
Copy link
Contributor Author

m1a2st commented Sep 19, 2024

Hello @chia7712,
The fail tests are:

  • OffsetsApiIntegrationTest#testAlterSinkConnectorOffsetsDifferentKafkaClusterTargeted the reason is can't met condition in 30s
  • PlaintextConsumerSubscriptionTest the reason is Failed to close kafka consumer

I think we could keep the 60s timeout for cluster test.

@mumrah
Copy link
Member

mumrah commented Sep 19, 2024

@m1a2st Is the PR title accurate? Does this add a global timeout for any test (@Test) or just our cluster tests?

@m1a2st
Copy link
Contributor Author

m1a2st commented Sep 19, 2024

@mumrah, Thanks for your reminder, we only add timeout for cluster tests in this PR, I should modify the title.

@m1a2st m1a2st changed the title KAFKA-16813: Add global timeout for "@Test" and "@TestTemplate" (For Github Action) KAFKA-16813: Add global timeout for "@TestTemplate" Sep 19, 2024
@chia7712
Copy link
Member

@m1a2st Do you forgot to add ClusterTest and ClusterTests to the title?

@m1a2st
Copy link
Contributor Author

m1a2st commented Sep 19, 2024

I consider the "@testtemplate" is a symbol for these annotations ClusterTest, ClusterTemplate, ClusterTests, but I will fix the title more clearly.

@m1a2st m1a2st changed the title KAFKA-16813: Add global timeout for "@TestTemplate" KAFKA-16813: Add global timeout for @ClusterTemplate and @ClusterTest and @ClusterTests Sep 19, 2024
@m1a2st m1a2st changed the title KAFKA-16813: Add global timeout for @ClusterTemplate and @ClusterTest and @ClusterTests KAFKA-16813: Add global timeout for @ClusterTemplate, @ClusterTest and @ClusterTests Sep 19, 2024
@chia7712 chia7712 merged commit 7975359 into apache:trunk Sep 20, 2024
8 of 9 checks passed
tedyu pushed a commit to tedyu/kafka that referenced this pull request Jan 6, 2025
@m1a2st @tedyu
KAFKA-16813 Add global timeout (60s) for @ClusterTemplate, `@Cluste…
f4c9048 
…rTest` and `@ClusterTests` (apache#16957)

Reviewers: TaiJuWu <[email protected]>, Chia-Ping Tsai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mumrah mumrah mumrah left review comments

@chia7712 chia7712 chia7712 approved these changes

@TaiJuWu TaiJuWu TaiJuWu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@m1a2st @chia7712 @mumrah @TaiJuWu