Skip to content

Conversation

@michaeljmarshall
Copy link
Member

@michaeljmarshall michaeljmarshall commented Aug 9, 2022

Fixes #17028 #16830

Motivation

In #16968, there was a change to remove some logic that appears necessary for certain message delivery guarantees. This PR fixes the minor regression introduced in that PR.

Modifications

  • Set sendInProgress = true before dispatching the read request.
  • Remove volatile keyword from sendInProgress. It is only updated within synchronized blocks.

Verifying this change

I verified that both testDelayedDeliveryWithMultipleConcurrentReadEntries and testMessageRedelivery pass consistently. Both were failing on my machine and stopped failing when I set sendInProgress = true.

Documentation

  • doc-not-needed

@michaeljmarshall
Copy link
Member Author

I removed the synchronized block because the two method calls are independent and do not need to happen with in the same lock (as far as I can tell).

@mattisonchao
Copy link
Member

Hi, this problem has already reverted at PR #17018

@michaeljmarshall
Copy link
Member Author

Thanks for pointing that out @mattisonchao. Closing this as it is superseded by #17018

@michaeljmarshall michaeljmarshall deleted the revert-part-of-16968 branch August 10, 2022 00:53
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 release/2.8.5 release/2.9.4 release/2.10.2 type/bug The PR fixed a bug or issue reported a bug type/flaky-tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky-test: DelayedDeliveryTest.testDelayedDeliveryWithMultipleConcurrentReadEntries

2 participants