Skip to content

Conversation

@heesung-sohn
Copy link
Contributor

@heesung-sohn heesung-sohn commented Mar 5, 2024

Motivation

Context: https://github.com/apache/pulsar/pull/22085/files#r1497008116

Currently, the connection pool key does not include physicalAddress (currently logicalAddress + keySuffix). This can be a problem when the same logicalAddresses are in the migrated(green) cluster. (the connection pool will return the connection to the old(blue) cluster)

Modifications

Add physicalAddress as part of the connection pool key

Verifying this change

  • Make sure that the change passes the CI checks.

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:

@github-actions
Copy link

github-actions bot commented Mar 5, 2024

@heesung-sn Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@heesung-sohn heesung-sohn self-assigned this Mar 5, 2024
@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Mar 5, 2024
heesung-sohn and others added 2 commits March 5, 2024 09:32
…ectionPool.java

Co-authored-by: Dragos Misca <dragosvictor@users.noreply.github.com>
…ectionPool.java

Co-authored-by: Dragos Misca <dragosvictor@users.noreply.github.com>
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

The Key looks good now. One more refactoring suggestion.

@heesung-sohn heesung-sohn changed the title [improve][broker] add physicalAddress as part of connection pool key [improve][client] add physicalAddress as part of connection pool key Mar 5, 2024
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

Good work @heesung-sn

@codecov-commenter
Copy link

codecov-commenter commented Mar 5, 2024

Codecov Report

❌ Patch coverage is 76.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.55%. Comparing base (bbc6224) to head (c716ce6).
⚠️ Report is 1260 commits behind head on master.

Files with missing lines Patch % Lines
.../org/apache/pulsar/client/impl/ConnectionPool.java 76.00% 5 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22196      +/-   ##
============================================
- Coverage     73.57%   73.55%   -0.02%     
- Complexity    32624    32653      +29     
============================================
  Files          1877     1878       +1     
  Lines        139502   139634     +132     
  Branches      15299    15328      +29     
============================================
+ Hits         102638   102711      +73     
- Misses        28908    28929      +21     
- Partials       7956     7994      +38     
Flag Coverage Δ
inttests 26.63% <48.00%> (+2.05%) ⬆️
systests 24.22% <48.00%> (-0.10%) ⬇️
unittests 72.84% <76.00%> (-0.01%) ⬇️

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

Files with missing lines Coverage Δ
.../org/apache/pulsar/client/impl/ConnectionPool.java 75.62% <76.00%> (+1.10%) ⬆️

... and 63 files with indirect coverage changes

🚀 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.

@heesung-sohn heesung-sohn merged commit e2f94dc into apache:master Mar 5, 2024
@heesung-sohn heesung-sohn deleted the cn-pool-key branch March 5, 2024 23:49
@heesung-sohn heesung-sohn added this to the 3.3.0 milestone Mar 6, 2024
@lhotari
Copy link
Member

lhotari commented Sep 17, 2025

@poorbarcode Do we need this fix in branch-3.0?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/client doc-not-needed Your PR changes do not impact docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants