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][test] Fix flaky test RawReaderTest #21008

Closed
wants to merge 13 commits into from

Conversation

Technoboy-
Copy link
Contributor

Motivation

https://github.com/apache/pulsar/actions/runs/5877094886/job/15945999791?pr=20990

Documentation

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

@Technoboy- Technoboy- marked this pull request as ready for review August 17, 2023 03:01
@Technoboy- Technoboy- self-assigned this Aug 17, 2023
@Technoboy- Technoboy- added this to the 3.2.0 milestone Aug 17, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 17, 2023
@Technoboy- Technoboy- changed the title [fix][test] Fix flaky test RawReaderTest.testHasMessageAvailableWithBatch [fix][test] Fix flaky test RawReaderTest Aug 17, 2023
@Technoboy- Technoboy- closed this Aug 17, 2023
@Technoboy- Technoboy- reopened this Aug 17, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2023

Codecov Report

Merging #21008 (b64ee22) into master (0cb1c78) will increase coverage by 38.92%.
Report is 3 commits behind head on master.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #21008       +/-   ##
=============================================
+ Coverage     33.56%   72.49%   +38.92%     
- Complexity    12198    32299    +20101     
=============================================
  Files          1621     1862      +241     
  Lines        126970   139445    +12475     
  Branches      13857    15379     +1522     
=============================================
+ Hits          42618   101090    +58472     
+ Misses        78748    30322    -48426     
- Partials       5604     8033     +2429     
Flag Coverage Δ
inttests ?
unittests 72.49% <100.00%> (+40.39%) ⬆️

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

Files Changed Coverage Δ
...a/org/apache/pulsar/client/impl/RawReaderImpl.java 83.80% <100.00%> (+1.11%) ⬆️
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 77.35% <100.00%> (+22.29%) ⬆️

... and 1496 files with indirect coverage changes

while (true) {
boolean hasMsg = reader.hasMessageAvailableAsync().get();
if (hasMsg && (messageCount == numKeys)) {
Assert.fail("HasMessageAvailable shows still has message when there is no message");
error.set(true);
Copy link
Member

Choose a reason for hiding this comment

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

Why do you change this? If you are using the Atomicboolean, you need to break after the set. Otherwise, this test will be blocked at reader.readNextAsync().get() because it shows there have messages but no message can be read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will revert this when find the root cause.

@Technoboy- Technoboy- closed this Aug 18, 2023
@Technoboy- Technoboy- reopened this Aug 18, 2023
@@ -321,7 +321,7 @@ public void testNegativeAcksDeleteFromUnackedTracker() throws Exception {
negativeAcksTracker.close();
}

@Test(timeOut = 10000)
@Test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Locally run takes about 8s sometimes, so remove this.

@@ -741,6 +741,7 @@ private void txnCumulativeAckTest(boolean batchEnable, int maxBatchSize, Subscri
for (int i = 0; i < messageCnt; i++){
producer.newMessage().value("hello".getBytes()).sendAsync();
}
producer.flush();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -529,7 +529,7 @@ protected void txnAckTest(boolean batchEnable, int maxBatchSize,
// after transaction abort, the messages could be received
Transaction commitTxn = getTxn();
for (int i = 0; i < messageCnt; i++) {
message = consumer.receive(2, TimeUnit.SECONDS);
message = consumer.receive();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the network is not good, here will fail. so no need to add timeout for this.
https://github.com/apache/pulsar/actions/runs/5899745435/job/16003227755?pr=21008

@Technoboy- Technoboy- closed this Aug 18, 2023
@Technoboy- Technoboy- reopened this Aug 18, 2023
@Technoboy- Technoboy- closed this Aug 18, 2023
@Technoboy- Technoboy- reopened this Aug 18, 2023
@Technoboy- Technoboy- closed this Aug 18, 2023
@Technoboy- Technoboy- reopened this Aug 18, 2023
@Technoboy- Technoboy- closed this Aug 18, 2023
@Technoboy- Technoboy- reopened this Aug 18, 2023
@@ -211,7 +211,7 @@ public void testReadMessageWithBatchingWithMessageInclusive() throws Exception {
reader.close();
}

@Test(timeOut = 10000)
@Test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Technoboy- Technoboy- closed this Aug 19, 2023
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 ready-to-test type/flaky-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants