Skip to content

[fix][broker] Fix invalid condition in logging exceptions#22412

Merged
lhotari merged 1 commit into
apache:masterfrom
teet-vaher-sympower:fix/remove_faulty_excl_marks
Apr 3, 2024
Merged

[fix][broker] Fix invalid condition in logging exceptions#22412
lhotari merged 1 commit into
apache:masterfrom
teet-vaher-sympower:fix/remove_faulty_excl_marks

Conversation

@teet-vaher-sympower

@teet-vaher-sympower teet-vaher-sympower commented Apr 3, 2024 •
edited by lhotari
Loading

Copy link
Copy Markdown
Contributor

Fixes #22409

Motivation

If Admin API request is sent to the broker and it responds with 307-redirect (for example in case of the bundle is not served by this broker) then ERROR is written into the broker log file. And there are quite a lot of these errors.

> GET https://current-pulsar-server:8443/admin/v2/non-persistent/tenant/namespace/0x00000000_0x40000000

< HTTP/1.1 307 Temporary Redirect
< Date: Tue, 02 Apr 2024 11:19:32 GMT
< Location: https://another-pulsar-server:8443/admin/v2/non-persistent/tenant/namespace/0x00000000_0x40000000?authoritative=false
< Content-Length: 0

broker log:

11:18:57.455 [pulsar-web-36-6] ERROR org.apache.pulsar.broker.admin.v2.NonPersistentTopics - [superuser] Failed to list topics on namespace bundle tenant/namespace/0x00000000_0x40000000
java.util.concurrent.CompletionException: javax.ws.rs.WebApplicationException: HTTP 307 Temporary Redirect
        at java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:315) ~[?:?]
...

Modifications

Issue #21995 introduced a new variable isNot307And404Exception. Previous code was substituted with the new variable in multiple places, but the exclamation mark in front of the variable (part of the previous code) was not removed. This PR removes the remaining unnecessary exclamation marks.

Verifying this change

  • Make sure that the change passes the CI checks.

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

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: teet-vaher-sympower#1

@github-actions

github-actions Bot commented Apr 3, 2024

Copy link
Copy Markdown

@teet-vaher-sympower 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 -->

@lhotari lhotari added this to the 3.3.0 milestone Apr 3, 2024
@github-actions github-actions Bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Apr 3, 2024
@lhotari lhotari changed the title [fix][broker] Remove faulty exclamation marks [fix][broker] Fix invalid condition in filtering exception logging Apr 3, 2024
@lhotari lhotari changed the title [fix][broker] Fix invalid condition in filtering exception logging [fix][broker] Fix invalid condition in logging exceptions Apr 3, 2024
@lhotari

lhotari commented Apr 3, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Great discovery. Thanks for the contribution @teet-vaher-sympower !

lhotari
lhotari approved these changes Apr 3, 2024
View reviewed changes

@lhotari lhotari 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.

LGTM

dao-jun
dao-jun approved these changes Apr 3, 2024
View reviewed changes

@dao-jun dao-jun 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.

lgtm

@lhotari lhotari merged commit cd6f53b into apache:master Apr 3, 2024
lhotari pushed a commit that referenced this pull request Apr 3, 2024
@teet-vaher-sympower @lhotari
[fix][broker] Fix invalid condition in logging exceptions (#22412)
a49e8a6 
(cherry picked from commit cd6f53b)
lhotari pushed a commit that referenced this pull request Apr 3, 2024
@teet-vaher-sympower @lhotari
[fix][broker] Fix invalid condition in logging exceptions (#22412)
afe4e18 
(cherry picked from commit cd6f53b)
lhotari pushed a commit that referenced this pull request Apr 3, 2024
@teet-vaher-sympower @lhotari
[fix][broker] Fix invalid condition in logging exceptions (#22412)
71ce55b 
(cherry picked from commit cd6f53b)
lhotari pushed a commit that referenced this pull request Apr 3, 2024
@teet-vaher-sympower @lhotari
[fix][broker] Fix invalid condition in logging exceptions (#22412)
5dac342 
(cherry picked from commit cd6f53b)
lhotari pushed a commit that referenced this pull request Apr 3, 2024
@teet-vaher-sympower @lhotari
[fix][broker] Fix invalid condition in logging exceptions (#22412)
265e4ed 
(cherry picked from commit cd6f53b)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 3, 2024
@teet-vaher-sympower @nikhil-ctds
[fix][broker] Fix invalid condition in logging exceptions (apache#22412)
86d3fe8 
(cherry picked from commit cd6f53b)
(cherry picked from commit 265e4ed)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 4, 2024
@teet-vaher-sympower @nikhil-ctds
[fix][broker] Fix invalid condition in logging exceptions (apache#22412)
cca9630 
(cherry picked from commit cd6f53b)
(cherry picked from commit 265e4ed)
nodece pushed a commit to ascentstream/pulsar that referenced this pull request May 13, 2024
@teet-vaher-sympower @nodece
[fix][broker] Fix invalid condition in logging exceptions (apache#22412)
3588062 
(cherry picked from commit cd6f53b)
@teet-vaher-sympower teet-vaher-sympower deleted the fix/remove_faulty_excl_marks branch May 21, 2024 11:05
hanmz pushed a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025
@teet-vaher-sympower @hanmz
[fix][broker] Fix invalid condition in logging exceptions (apache#22412)
0308b23
@codecov-commenter

codecov-commenter commented Apr 3, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 10.20408% with 44 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.72%. Comparing base (bbc6224) to head (17b1d07).
⚠️ Report is 1767 commits behind head on master.

Files with missing lines Patch % Lines
...pulsar/broker/admin/impl/PersistentTopicsBase.java 12.50% 15 Missing and 20 partials ⚠️
...rg/apache/pulsar/broker/admin/v3/Transactions.java 0.00% 0 Missing and 6 partials ⚠️
...he/pulsar/broker/admin/v2/NonPersistentTopics.java 0.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22412      +/-   ##
============================================
+ Coverage     73.57%   73.72%   +0.15%     
+ Complexity    32624    32290     -334     
============================================
  Files          1877     1885       +8     
  Lines        139502   139881     +379     
  Branches      15299    15321      +22     
============================================
+ Hits         102638   103132     +494     
+ Misses        28908    28775     -133     
- Partials       7956     7974      +18
Flag Coverage Δ
inttests 27.01% <0.00%> (+2.42%) ⬆️
systests 24.61% <0.00%> (+0.29%) ⬆️
unittests 72.98% <10.20%> (+0.14%) ⬆️

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

Files with missing lines Coverage Δ
...he/pulsar/broker/admin/v2/NonPersistentTopics.java 61.00% <0.00%> (-0.46%) ⬇️
...rg/apache/pulsar/broker/admin/v3/Transactions.java 67.80% <0.00%> (-8.22%) ⬇️
...pulsar/broker/admin/impl/PersistentTopicsBase.java 68.04% <12.50%> (+2.58%) ⬆️

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

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

Reviewers

@lhotari lhotari lhotari approved these changes

@dao-jun dao-jun dao-jun approved these changes

@poorbarcode poorbarcode poorbarcode approved these changes

@Technoboy- Technoboy- Awaiting requested review from Technoboy-

Assignees

@teet-vaher-sympower teet-vaher-sympower

Labels

cherry-picked/branch-2.10 cherry-picked/branch-2.11 cherry-picked/branch-3.0 cherry-picked/branch-3.1 cherry-picked/branch-3.2 doc-not-needed Your PR changes do not impact docs ready-to-test release/2.10.7 release/2.11.5 release/3.0.5 release/3.1.4 release/3.2.3 type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Milestone

3.3.0

Development

Successfully merging this pull request may close these issues.

[Bug][broker] Broker contains invalid condition for logging exceptions

5 participants

@teet-vaher-sympower @lhotari @codecov-commenter @dao-jun @poorbarcode