Skip to content

Fix UnAckedMessageTracker timeout send redelivery messages - #9134

Closed
saosir wants to merge 1 commit into
apache:masterfrom
saosir:patch-5
Closed

Fix UnAckedMessageTracker timeout send redelivery messages#9134
saosir wants to merge 1 commit into
apache:masterfrom
saosir:patch-5

Conversation

@saosir

@saosir saosir commented Jan 6, 2021

Copy link
Copy Markdown
Contributor

Motivation

pulsar client UnAckedMessageTracker wait one more tickDurationInMs to send redelivery message at timeout

Modifications

Fix the formula for calculating the size of timePartitions through the parameters ackTimeoutMillis and tickDurationInMs

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

  • Does this pull request introduce a new feature? (no)

@sijie

sijie commented Jan 6, 2021

Copy link
Copy Markdown
Member

@BewareMyPower Can you review this pull request?

@saosir

saosir commented Jan 6, 2021

Copy link
Copy Markdown
Contributor Author

see #9072 (comment)

@sijie

sijie commented Jan 6, 2021

Copy link
Copy Markdown
Member

@saosir Can you rebase to the latest master? There are recent changes to fix the broken master.

@BewareMyPower

Copy link
Copy Markdown
Contributor

/pulsarbot run-failure-checks

1 similar comment
@saosir

saosir commented Jan 8, 2021

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@BewareMyPower

Copy link
Copy Markdown
Contributor

/pulsarbot run-failure-checks

@eolivelli

Copy link
Copy Markdown
Contributor

What about extracting the computation to a static method and create unit tests?
Even of it is a one line change it is not so obvious that the fix works

@saosir

saosir commented Jan 21, 2021

Copy link
Copy Markdown
Contributor Author

Why the CI always fail? @BewareMyPower

@BewareMyPower

Copy link
Copy Markdown
Contributor

@saosir Maybe you can try to rebase to latest master because sometimes some flaky tests might be fixed. (I also noticed this PR's test failed for many times...

@BewareMyPower

Copy link
Copy Markdown
Contributor

/pulsarbot run-failure-checks

1 similar comment
@saosir

saosir commented Jan 27, 2021

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@saosir
saosir force-pushed the patch-5 branch 2 times, most recently from 2181a14 to 5629357 Compare February 3, 2021 07:24
@sijie

sijie commented Feb 9, 2021

Copy link
Copy Markdown
Member

/pulsarbot run-failure-checks

2 similar comments
@sijie

sijie commented Feb 11, 2021

Copy link
Copy Markdown
Member

/pulsarbot run-failure-checks

@sijie

sijie commented Feb 16, 2021

Copy link
Copy Markdown
Member

/pulsarbot run-failure-checks

@zymap

zymap commented Feb 23, 2021

Copy link
Copy Markdown
Member

/pulsarbot run-failure-checks

1 similar comment
@saosir

saosir commented Feb 24, 2021

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@zymap

zymap commented Feb 24, 2021

Copy link
Copy Markdown
Member

Failure tests

[INFO] 
Error:  Failures: 
Error:  org.apache.pulsar.client.impl.PerMessageUnAcknowledgedRedeliveryTest.testSharedAckedNormalTopic(org.apache.pulsar.client.impl.PerMessageUnAcknowledgedRedeliveryTest)
[INFO]   Run 1: PASS
Error:    Run 2: PerMessageUnAcknowledgedRedeliveryTest.testSharedAckedNormalTopic:148 expected [5] but found [10]
[INFO] 
Error:  org.apache.pulsar.client.impl.PerMessageUnAcknowledgedRedeliveryTest.testSharedAckedPartitionedTopic(org.apache.pulsar.client.impl.PerMessageUnAcknowledgedRedeliveryTest)
[INFO]   Run 1: PASS
Error:    Run 2: PerMessageUnAcknowledgedRedeliveryTest.testSharedAckedPartitionedTopic:494 expected [5] but found [10]
[INFO] 
[INFO] 
Error:  Tests run: 229, Failures: 2, Errors: 0, Skipped: 0

@zymap

zymap commented Feb 25, 2021

Copy link
Copy Markdown
Member

@saosir Looks like the CI failed by this change. Please take a look. Thank you.

I will push this to the next release.

@saosir

saosir commented Feb 25, 2021

Copy link
Copy Markdown
Contributor Author

@saosir Looks like the CI failed by this change. Please take a look. Thank you.

I will push this to the next release.

OK, I am looking for time to fix this problem. Before I thought that modifying two lines of code would not have any impact on
test cases, but this caused some cases to fail.

@saosir

saosir commented Feb 25, 2021

Copy link
Copy Markdown
Contributor Author

see #3118
@codelipenghui pulsar client UnAckedMessageTracker wait one more tickDurationInMs to send redelivery message at timeout. Is it expected?The test case is also designed like this.

assertEquals(size, 10);
Thread.sleep(ackTimeOutMillis);
// 10. Receiver receives redelivered messages
message = consumer.receive();
int redelivered = 0;
while (message != null) {
redelivered++;
String data = new String(message.getData());
log.info("Consumer received : " + data);
consumer.acknowledge(message);
message = consumer.receive(100, TimeUnit.MILLISECONDS);
}
assertEquals(redelivered, 5);
size = getUnackedMessagesCountInPartitionedConsumer(consumer);
log.info(key + " Unacked Message Tracker size is " + size);
assertEquals(size, 5);

How about just close this pull request?

@saosir saosir closed this Mar 1, 2021
@saosir saosir reopened this Mar 1, 2021
@saosir saosir closed this Mar 1, 2021
@saosir
saosir deleted the patch-5 branch May 26, 2021 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants