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.

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

test: Fix and improve the test QuickDegradeAndRestoreCommandTopicIntegrationTest #8275

Merged

Conversation

mkandaswamy
Copy link
Member

@mkandaswamy mkandaswamy commented Oct 21, 2021

Description

KSQL-7430
Improve the test QuickDegradeAndRestoreCommandTopicIntegrationTest by checking for topic deletion status via assertThatEventually() before checking for degraded state and also after right after deleting the command topic.

Also, fixed a race which lead to automatic recreation of deleted command topic by setting Kafka config: auto.create.topics.enable to be false in QuickDegradeAndRestoreCommandTopicIntegrationTest.

Testing done

Tested the change locally by running integration tests multiple times .

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

…est by checking for topic deletion status via assertThatEventually() before checking for degraded state. If the command topic gets recreated somehow and is the reason behind the flakiness, this change helps to isolate that.
…Test which lead to automatic recreation of deleted command topic by setting Kafka config: auto.create.topics.enable to false. Also, added another check to ensure command topic is deleted before stopping ksqldb.
@mkandaswamy mkandaswamy changed the title test: Improve the test QuickDegradeAndRestoreCommandTopicIntegrationTest test: Fix and improve the test QuickDegradeAndRestoreCommandTopicIntegrationTest Oct 21, 2021
@mkandaswamy mkandaswamy marked this pull request as ready for review October 21, 2021 20:03
@mkandaswamy mkandaswamy requested a review from a team as a code owner October 21, 2021 20:03
Copy link
Member

@nateab nateab left a comment

Choose a reason for hiding this comment

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

Besides changing the auto topic creation property to be false for just this test, LGTM otherwise (I think it should also help you get a green build on jenkins)! Thank you for the quick fix @mkandaswamy!

config.put(KafkaConfig.AutoCreateTopicsEnableProp(), true);
// Create topics explicitly when needed to avoid a race which
// automatically recreates deleted command topic:
config.put(KafkaConfig.AutoCreateTopicsEnableProp(), false);
Copy link
Member

Choose a reason for hiding this comment

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

I'm worried that there may be many tests that rely on automatically creating topics. Is there way to set this config but at the test level instead?

Copy link
Member

@nateab nateab Oct 21, 2021

Choose a reason for hiding this comment

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

If you look at ApiIntegrationTest, when it creates the TEST_HARNESS it configure the properties it needs for the kafka cluster as follows:

private static final IntegrationTestHarness TEST_HARNESS = IntegrationTestHarness.builder()
      
      .withKafkaCluster(

          EmbeddedSingleNodeKafkaCluster.newBuilder()

              .withoutPlainListeners()

              .withSaslSslListeners()
      ).build();

Can we do something similar here and create a function to remove the auto topic creation for just this test?

Copy link
Member Author

@mkandaswamy mkandaswamy Oct 21, 2021

Choose a reason for hiding this comment

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

Yes, seem like few tests do depend on that setting to be true. I've addressed this in my latest commit. For now, only QuickDegradeAndRestoreCommandTopicIntegrationTest will set automatically creating topics to be false.

REST_APP.stop();
REST_APP.start();

// Then
assertThatEventually("Topic Deleted", this::isCommandTopicDeleted, is(true));
Copy link
Member

Choose a reason for hiding this comment

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

good check

…e as few tests depend on that setting.

Also, added new method withoutAutoCreateTopics() to create Kafka cluster with auto create topics feature turned off.
This method is used by QuickDegradeAndRestoreCommandTopicIntegrationTest to fix a race which lead to automatic recreation
of deleted command topic.
@mkandaswamy mkandaswamy merged commit badaf17 into confluentinc:master Oct 22, 2021
Copy link
Member

@stevenpyzhang stevenpyzhang left a comment

Choose a reason for hiding this comment

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

lgtm

@mkandaswamy mkandaswamy deleted the improve_integration_test_qdarct branch October 22, 2021 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants