Skip to content

Conversation

@lhotari
Copy link
Member

@lhotari lhotari commented Nov 11, 2025

Motivation

BinaryProtoLookupService.getTopicsUnderNamespace method contains retry logic, but it only applies to exceptions that occur when acquiring the connection fails.
A getTopicsUnderNamespace might fail later if the connection gets closed before the response has been successfully received. That's why it's better that the retry logic would also cover the actual operation to be effective.

Modifications

  • Fix the retry logic and add tests

Documentation

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

@lhotari lhotari added this to the 4.2.0 milestone Nov 11, 2025
@lhotari lhotari added type/bug The PR fixed a bug or issue reported a bug area/client ready-to-test labels Nov 11, 2025
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 11, 2025
@lhotari lhotari force-pushed the lh-fix-gettopics-retries branch from 2da65d4 to 2562c19 Compare November 11, 2025 16:10
@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2025

Codecov Report

❌ Patch coverage is 89.36170% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.28%. Comparing base (f29ca21) to head (b3ab4f0).
⚠️ Report is 20 commits behind head on master.

Files with missing lines Patch % Lines
...e/pulsar/client/impl/BinaryProtoLookupService.java 86.84% 2 Missing and 3 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #24974       +/-   ##
=============================================
+ Coverage     38.69%   74.28%   +35.58%     
- Complexity    13362    34017    +20655     
=============================================
  Files          1863     1920       +57     
  Lines        145975   150236     +4261     
  Branches      16928    17428      +500     
=============================================
+ Hits          56487   111604    +55117     
+ Misses        81858    29719    -52139     
- Partials       7630     8913     +1283     
Flag Coverage Δ
inttests 26.32% <12.76%> (+0.03%) ⬆️
systests 22.86% <40.42%> (-0.01%) ⬇️
unittests 73.81% <89.36%> (+38.82%) ⬆️

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

Files with missing lines Coverage Δ
.../java/org/apache/pulsar/client/impl/ClientCnx.java 69.33% <100.00%> (+15.64%) ⬆️
...rg/apache/pulsar/client/impl/PulsarClientImpl.java 75.55% <100.00%> (+18.14%) ⬆️
...pache/pulsar/common/protocol/FrameDecoderUtil.java 100.00% <100.00%> (ø)
...e/pulsar/client/impl/BinaryProtoLookupService.java 83.39% <86.84%> (+12.41%) ⬆️

... and 1416 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.

private final Queue<Object> queue = Queues.newArrayDeque();

public ClientChannelHelper() {
int maxMessageSize = 5 * 1024 * 1024;
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed.

}, lookupPinnedExecutor).whenComplete((r, t) -> {
if (t != null) {
Throwable cause = FutureUtil.unwrapCompletionException(t);
if (cause instanceof PulsarClientException && !PulsarClientException.isRetriableError(cause)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it covered in the test here?

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 ready-to-test 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.

3 participants