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

[fix][flaky-test] fix flaky test PerformanceTransactionTest #18296

Merged
merged 2 commits into from
Nov 3, 2022

Conversation

labuladong
Copy link
Contributor

@labuladong labuladong commented Nov 2, 2022

Fixes #18282

Modifications

We start 10 transactions and consume 50 messages in every transaction by default.

But notice -r 10 set the consumption rate limit to be 10 msg per second, so every transaction is about 5 seconds.

In #17837 we set the transaction TTL to be 5 seconds, so transactions may be timeout randomly, which causes the test to fail.

To prove this, you can try to set -r 9 or -tto 4, which will cause failure 100%.

So remove the -r option can fix this issue.

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: labuladong#6

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 2, 2022
@codelipenghui
Copy link
Contributor

@poorbarcode Please also help review this PR.

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.

But notice -r 10 set the consumption rate limit to be 10 msg per second, so every transaction is about 5 seconds. In #17837 we set the transaction TTL to be 5 seconds

Yes, that is the root cause. Each transaction cost 5s and the timeout is 5s.

@liangyepianzhou Do we need the slow commit here?

If need that,. we can set -r 50 to resolve this problem.

@liangyepianzhou
Copy link
Contributor

@poorbarcode Good idea, I also prefer to modify it to -r 50 which can make the test more comprehensive.
And @labuladong Could you please also fix testProduceTxnMessage for the same problem?

@labuladong
Copy link
Contributor Author

OK, I'll do this @liangyepianzhou

@codecov-commenter
Copy link

codecov-commenter commented Nov 3, 2022

Codecov Report

Merging #18296 (b26c45d) into master (0866c3a) will increase coverage by 13.06%.
The diff coverage is 47.88%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #18296       +/-   ##
=============================================
+ Coverage     38.97%   52.03%   +13.06%     
+ Complexity     8311     7419      -892     
=============================================
  Files           683      400      -283     
  Lines         67325    43682    -23643     
  Branches       7217     4479     -2738     
=============================================
- Hits          26239    22730     -3509     
+ Misses        38079    18512    -19567     
+ Partials       3007     2440      -567     
Flag Coverage Δ
unittests 52.03% <47.88%> (+13.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../main/java/org/apache/pulsar/PulsarStandalone.java 0.00% <ø> (ø)
...che/pulsar/broker/BookKeeperClientFactoryImpl.java 0.00% <0.00%> (ø)
...pulsar/broker/admin/impl/PersistentTopicsBase.java 59.28% <0.00%> (+6.98%) ⬆️
...pache/pulsar/broker/admin/v2/PersistentTopics.java 73.96% <0.00%> (+3.17%) ⬆️
...pulsar/broker/service/PulsarCommandSenderImpl.java 73.29% <ø> (-1.07%) ⬇️
...ersistentStreamingDispatcherMultipleConsumers.java 0.00% <0.00%> (ø)
...ransaction/buffer/impl/InMemTransactionBuffer.java 0.00% <ø> (ø)
...nsaction/buffer/impl/TransactionBufferDisable.java 52.63% <ø> (ø)
.../metadata/v2/TransactionBufferSnapshotIndexes.java 0.00% <0.00%> (ø)
...a/v2/TransactionBufferSnapshotIndexesMetadata.java 0.00% <0.00%> (ø)
... and 356 more

@labuladong
Copy link
Contributor Author

PTAL @liangyepianzhou

@congbobo184 congbobo184 changed the title [fix][test] PerformanceTransactionTest [fix][flaky-test] PerformanceTransactionTest Nov 3, 2022
@congbobo184 congbobo184 changed the title [fix][flaky-test] PerformanceTransactionTest [fix][flaky-test] fix flaky test PerformanceTransactionTest Nov 3, 2022
@congbobo184 congbobo184 merged commit 3cb9d4d into apache:master Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs type/flaky-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PerformanceTransactionTest.testConsumeTxnMessage
6 participants