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][broker] make closing producer thread-safe while updating recently closed producer #21355

Merged
merged 1 commit into from
Oct 14, 2023

Conversation

rdhabalia
Copy link
Contributor

@rdhabalia rdhabalia commented Oct 13, 2023

Motivation

Right now, #20747 has a limitation where closing producer has to be called from the same executor of the connection-ctx which might not be always true for usecases where async operation can be completed by a different thread. #20747 was added to store value in closed-producer map thread safely so, the correct fix is to make the map thread-safe to avoid duplicate efforts during storing into the map which will remove the above limitation.

Modifications

Make recentlyClosedProducers thread-safe as it can be accessed by multiple threads.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

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

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository:

@rdhabalia rdhabalia added area/broker doc-not-needed Your PR changes do not impact docs ready-to-test labels Oct 13, 2023
@rdhabalia rdhabalia self-assigned this Oct 13, 2023
@lhotari lhotari closed this Oct 14, 2023
@lhotari lhotari reopened this Oct 14, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2023

Codecov Report

Merging #21355 (29d02b2) into master (1a352f1) will increase coverage by 39.83%.
Report is 3 commits behind head on master.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #21355       +/-   ##
=============================================
+ Coverage     33.49%   73.32%   +39.83%     
- Complexity    12257    32568    +20311     
=============================================
  Files          1636     1888      +252     
  Lines        127789   140291    +12502     
  Branches      13963    15455     +1492     
=============================================
+ Hits          42798   102868    +60070     
+ Misses        79372    29361    -50011     
- Partials       5619     8062     +2443     
Flag Coverage Δ
inttests 24.27% <100.00%> (+0.13%) ⬆️
systests 24.68% <100.00%> (?)
unittests 72.62% <100.00%> (+40.61%) ⬆️

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

Files Coverage Δ
...va/org/apache/pulsar/broker/service/ServerCnx.java 72.00% <100.00%> (+32.48%) ⬆️

... and 1531 files with indirect coverage changes

@lhotari lhotari merged commit a5f4c1e into apache:master Oct 14, 2023
83 of 87 checks passed
@rdhabalia rdhabalia deleted the producer_close_thread branch October 14, 2023 18:45
vraulji567 pushed a commit to vraulji567/pulsar that referenced this pull request Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants