Skip to content

[improve][test] Improve SimpleProducerConsumerTest to reduce the execution time #17887

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
Merged
codelipenghui merged 3 commits into from
Oct 11, 2022
Merged

[improve][test] Improve SimpleProducerConsumerTest to reduce the execution time #17887

codelipenghui merged 3 commits into from
Oct 11, 2022

Conversation

@coderzc
Copy link
Member

@coderzc coderzc commented Sep 29, 2022 •
edited
Loading

Fixed

Modifications

Only execution once setup/clean and clean tenants/namespaces/producer/consumer after the end of each test execution.

Execution time after improve test:

Tests run: 102, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 461.255 s - in org.apache.pulsar.client.api.SimpleProducerConsumerTest

Tests run: 102, Failures: 0, Errors: 0, Skipped: 1, Time elapsed: 434.111 s - in org.apache.pulsar.broker.service.persistent.SimpleProducerConsumerTestStreamingDispatcherTest

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

Matching PR in forked repository

PR in forked repository: coderzc#7

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 29, 2022
poorbarcode
poorbarcode reviewed Sep 29, 2022
View reviewed changes
Copy link
Contributor

@poorbarcode poorbarcode left a comment

Choose a reason for hiding this comment

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

pulsar/pulsar-broker/src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java

Line 2814 in afd2949

this.conf.setMaxMessageSize(1000);

I think this conf should be reset after method

pulsar-broker/src/test/java/org/apache/pulsar/client/api/SimpleProducerConsumerTest.java Outdated

for (String tenant : admin.tenants().getTenants()) {
for (String namespace : admin.namespaces().getNamespaces(tenant)) {
admin.namespaces().deleteNamespace(namespace, true);
Copy link
Contributor

@poorbarcode poorbarcode Sep 29, 2022

Choose a reason for hiding this comment

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

Suggest to use deleteNamespaceGraceFully, because delete namespace is not a stable cmd, see #17157.

Note that deleteNamespaceGraceFully runs slower than delete namespace and is unsuitable for all scenarios.

Copy link
Member Author

@coderzc coderzc Sep 30, 2022

Choose a reason for hiding this comment

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

Ok, thanks.

Copy link
Contributor

@ethqunzhong ethqunzhong Oct 14, 2022

Choose a reason for hiding this comment

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

Suggest to use deleteNamespaceGraceFully, because delete namespace is not a stable cmd, see #17157.

Note that deleteNamespaceGraceFully runs slower than delete namespace and is unsuitable for all scenarios.

in my CI test, There are two failures related to these changes:
Time elapsed failures while excute deleteNamespaceGraceFully
see the checks in https://github.com/apache/pulsar/actions/runs/3242148385/jobs/5326408439
could i skip this test temporary？

Copy link
Contributor

@ethqunzhong ethqunzhong Oct 17, 2022

Choose a reason for hiding this comment

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

@poorbarcode hi
my CI check always fails in Pulsar CI Flaky / Flaky tests suite . seem releted to run deleteNamespaceGraceFully
cloud you please help me confirm the reason? thx
https://github.com/apache/pulsar/actions/runs/3242148385/jobs/5359655987

Copy link
Contributor

@poorbarcode poorbarcode Oct 17, 2022

Choose a reason for hiding this comment

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

@ethqunzhong I see, could you rebase master and see if it reproduces?

Copy link
Contributor

@poorbarcode poorbarcode Oct 17, 2022

Choose a reason for hiding this comment

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

Hi @ethqunzhong If you said PR #17803, then the error does not affect the merge

Copy link
Contributor

@ethqunzhong ethqunzhong Oct 17, 2022

Choose a reason for hiding this comment

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

@poorbarcode thx your reply.

@poorbarcode poorbarcode mentioned this pull request Sep 30, 2022
Closed
14 tasks
@coderzc coderzc force-pushed the slow/SimpleProducerConsumerTest branch from afd2949 to 6a550af Compare September 30, 2022 02:45
@coderzc
Copy link
Member Author

coderzc commented Oct 8, 2022

@codelipenghui PTAL.

@codelipenghui codelipenghui added this to the 2.12.0 milestone Oct 10, 2022
@codelipenghui codelipenghui reopened this Oct 10, 2022
@codelipenghui codelipenghui merged commit a4971f5 into apache:master Oct 11, 2022
@nicoloboschi
Copy link
Contributor

nicoloboschi commented Oct 14, 2022

@coderzc @codelipenghui there were failures in the CI for this pull in the flaky test suite. See the checks https://github.com/apache/pulsar/actions/runs/3219150517

There are two failures related to these changes:

  • SimpleProducerConsumerTest.rest
    Cannot invoke "org.apache.pulsar.broker.PulsarService.getConfiguration()" because "this.pulsar" is null
  • SimpleProducerConsumerTest.testConcurrentConsumerReceiveWhileReconnect
    Cannot invoke "org.apache.pulsar.client.api.PulsarClient.newConsumer()" because "this.pulsarClient" is null

Could you please address them?

@ethqunzhong ethqunzhong mentioned this pull request Oct 14, 2022
Merged
4 tasks
@nicoloboschi nicoloboschi mentioned this pull request Oct 17, 2022
Merged
5 tasks
@Denovo1998 Denovo1998 mentioned this pull request Apr 14, 2024
Merged
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@poorbarcode poorbarcode poorbarcode approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

+1 more reviewer

@ethqunzhong ethqunzhong ethqunzhong left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@coderzc coderzc

Labels

area/test doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@coderzc @nicoloboschi @codelipenghui @ethqunzhong @poorbarcode