Skip to content

[fix][broker] Fix the order of resource close in the InMemoryDelayedDeliveryTracker - #18000

Merged
codelipenghui merged 2 commits into
apache:masterfrom
coderzc:fix_InMemoryDelayedDeliveryTracker_close
Oct 13, 2022
Merged

[fix][broker] Fix the order of resource close in the InMemoryDelayedDeliveryTracker#18000
codelipenghui merged 2 commits into
apache:masterfrom
coderzc:fix_InMemoryDelayedDeliveryTracker_close

Conversation

@coderzc

@coderzc coderzc commented Oct 11, 2022

Copy link
Copy Markdown
Member

Motivation

In the InMemoryDelayedDeliveryTracker, the priorityQueue should be closed after the timeout task is canceled, otherwise, we may operate the priorityQueue that has been closed.

Modifications

Close the priorityQueue after the timeout task is canceled and set timeout=null.

Documentation

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

Matching PR in forked repository

PR in forked repository: coderzc#11

@mattisonchao mattisonchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we add a test for it?

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lgtm

@codelipenghui codelipenghui added this to the 2.12.0 milestone Oct 12, 2022
@codecov-commenter

codecov-commenter commented Oct 12, 2022

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (master@9281bbd). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...broker/delayed/InMemoryDelayedDeliveryTracker.java 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #18000   +/-   ##
=========================================
  Coverage          ?   45.74%           
  Complexity        ?    17517           
=========================================
  Files             ?     1573           
  Lines             ?   128189           
  Branches          ?    14093           
=========================================
  Hits              ?    58642           
  Misses            ?    63485           
  Partials          ?     6062           
Flag Coverage Δ
unittests 45.74% <75.00%> (?)

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

Files with missing lines Coverage Δ
...broker/delayed/InMemoryDelayedDeliveryTracker.java 65.00% <75.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderzc

coderzc commented Oct 12, 2022

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

@coderzc
coderzc force-pushed the fix_InMemoryDelayedDeliveryTracker_close branch from 8d09402 to bbba3bf Compare October 12, 2022 08:51
@coderzc

coderzc commented Oct 12, 2022

Copy link
Copy Markdown
Member Author

Can we add a test for it?

I add testClose method to cover this case.

@coderzc

coderzc commented Oct 13, 2022

Copy link
Copy Markdown
Member Author

/pulsarbot run-failure-checks

@codelipenghui
codelipenghui merged commit 44ae348 into apache:master Oct 13, 2022
congbobo184 pushed a commit that referenced this pull request Nov 14, 2022
@congbobo184 congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 14, 2022
congbobo184 pushed a commit that referenced this pull request Nov 26, 2022
liangyepianzhou pushed a commit that referenced this pull request Dec 6, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Dec 6, 2022
…eliveryTracker (apache#18000)

(cherry picked from commit 44ae348)
(cherry picked from commit a9c67fa)
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.

8 participants