Skip to content

Conversation

BewareMyPower
Copy link
Contributor

Motivation

When the lookup properties are modified, the previous pending lookup might use the modified properties for lookup.

Modifications

  • Combine the properties and the topic name as the key to cache pending lookup requests.
  • Return copied properties rather than the reference in ClientConfigurationData#getProperties
  • Add testConcurrentLookupProperties to cover this case.

Documentation

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

Matching PR in forked repository

PR in forked repository:

@BewareMyPower BewareMyPower self-assigned this Sep 5, 2024
@BewareMyPower BewareMyPower added the type/bug The PR fixed a bug or issue reported a bug label Sep 5, 2024
@BewareMyPower BewareMyPower added this to the 4.0.0 milestone Sep 5, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 5, 2024
@codecov-commenter
Copy link

codecov-commenter commented Sep 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.53%. Comparing base (bbc6224) to head (8e48dc0).
Report is 566 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23260      +/-   ##
============================================
+ Coverage     73.57%   74.53%   +0.96%     
- Complexity    32624    34292    +1668     
============================================
  Files          1877     1926      +49     
  Lines        139502   145043    +5541     
  Branches      15299    15862     +563     
============================================
+ Hits         102638   108111    +5473     
+ Misses        28908    28683     -225     
- Partials       7956     8249     +293     
Flag Coverage Δ
inttests 27.96% <100.00%> (+3.38%) ⬆️
systests 24.63% <100.00%> (+0.31%) ⬆️
unittests 73.89% <100.00%> (+1.05%) ⬆️

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

Files with missing lines Coverage Δ
...e/pulsar/client/impl/BinaryProtoLookupService.java 81.69% <100.00%> (-0.85%) ⬇️

... and 561 files with indirect coverage changes

@BewareMyPower BewareMyPower merged commit 1439529 into apache:master Sep 6, 2024
58 of 59 checks passed
@BewareMyPower BewareMyPower deleted the bewaremypower/concurrent-lookup-properties branch September 6, 2024 04:02
michalcukierman pushed a commit to michalcukierman/pulsar that referenced this pull request Sep 11, 2024
hanmz pushed a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025
@lhotari
Copy link
Member

lhotari commented Mar 4, 2025

@BewareMyPower Is this a bug fix that should be cherry-picked to branch-3.0?

@BewareMyPower
Copy link
Contributor Author

No, the lookup properties feature was introduced from PIP-368 (#23075), which is only included in 4.0 or later.

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 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants