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.

Sign up for GitHub

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

Jump to bottom

[fix][broker] Fix namespace not found will cause request timeout #18512

Merged
merged 3 commits into from
Nov 28, 2022
Merged

[fix][broker] Fix namespace not found will cause request timeout #18512

merged 3 commits into from
Nov 28, 2022

Conversation

labuladong
Copy link
Contributor

Fixes #18497

Modifications

internalGetSubscriptions didn't handle namespace not found exception so the web request won't get response, which causes request timout.

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

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

Matching PR in forked repository

PR in forked repository: labuladong#13

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 17, 2022
eolivelli
eolivelli requested changes Nov 17, 2022
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

good catch!

Could you please add a test case that covers this fix ?

@labuladong
Copy link
Contributor Author

OK, I will add a test.

@labuladong labuladong requested a review from eolivelli November 18, 2022 12:18
@nodece nodece added this to the 2.12.0 milestone Nov 21, 2022
@nodece
Copy link
Member

nodece commented Nov 24, 2022

/pulsarbot rerun-failure-checks

@codecov-commenter
Copy link

codecov-commenter commented Nov 24, 2022 •
edited
Loading

Codecov Report

Merging #18512 (911f8bf) into master (598ca5d) will decrease coverage by 0.85%.
The diff coverage is 80.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18512      +/-   ##
============================================
- Coverage     47.72%   46.87%   -0.86%     
- Complexity     9330    10347    +1017     
============================================
  Files           618      698      +80     
  Lines         58568    68075    +9507     
  Branches       6093     7279    +1186     
============================================
+ Hits          27951    31909    +3958     
- Misses        27589    32579    +4990     
- Partials       3028     3587     +559
Flag Coverage Δ
unittests 46.87% <80.00%> (-0.86%) ⬇️

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

Impacted Files Coverage Δ
...pulsar/broker/admin/impl/PersistentTopicsBase.java 59.25% <80.00%> (+0.63%) ⬆️
...ion/buffer/metadata/TransactionBufferSnapshot.java 42.85% <0.00%> (-42.86%) ⬇️
...ker/service/schema/exceptions/SchemaException.java 60.00% <0.00%> (-40.00%) ⬇️
...lsar/broker/loadbalance/impl/ThresholdShedder.java 3.26% <0.00%> (-39.87%) ⬇️
...saction/timeout/TransactionTimeoutTrackerImpl.java 50.87% <0.00%> (-36.85%) ⬇️
...er/impl/SingleSnapshotAbortedTxnProcessorImpl.java 54.76% <0.00%> (-19.05%) ⬇️
.../apache/pulsar/broker/loadbalance/LoadManager.java 61.11% <0.00%> (-16.67%) ⬇️
...nsaction/pendingack/impl/PendingAckHandleImpl.java 36.76% <0.00%> (-15.08%) ⬇️
...ar/broker/transaction/util/LogIndexLagBackoff.java 42.85% <0.00%> (-14.29%) ⬇️
...ransaction/buffer/impl/TopicTransactionBuffer.java 44.18% <0.00%> (-13.96%) ⬇️
... and 136 more

@labuladong
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

1 similar comment
@labuladong
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@nodece
Copy link
Member

nodece commented Nov 25, 2022

@eolivelli Could you review this PR again?

@labuladong
Copy link
Contributor Author

Can this pr be merged?

@nodece
Copy link
Member

nodece commented Nov 28, 2022

Can this pr be merged?

Please wait for @eolivelli.

eolivelli
eolivelli approved these changes Nov 28, 2022
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Lgtm

@eolivelli eolivelli merged commit 7b06489 into apache:master Nov 28, 2022
@labuladong labuladong deleted the fix-admin-cli-sub branch November 29, 2022 06:26
@Technoboy- Technoboy- changed the title [fix][broker] namespace not found will cause request timeout [fix][broker] Fix namespace not found will cause request timeout Dec 5, 2022
lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Dec 9, 2022
@labuladong @lifepuzzlefun
[fix][broker] namespace not found will cause request timeout (apache#…
9b14468 
…18512)
lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Jan 10, 2023
@labuladong @lifepuzzlefun
[fix][broker] namespace not found will cause request timeout (apache#…
2d67aa4 
…18512)
poorbarcode pushed a commit that referenced this pull request Nov 13, 2023
@labuladong @poorbarcode
[fix][broker] namespace not found will cause request timeout (#18512)
43a19b2 
(cherry picked from commit 7b06489)
poorbarcode pushed a commit that referenced this pull request Nov 13, 2023
@labuladong @poorbarcode
[fix][broker] namespace not found will cause request timeout (#18512)
18cffc5 
(cherry picked from commit 7b06489)
nodece pushed a commit to ascentstream/pulsar that referenced this pull request Dec 2, 2023
@labuladong @nodece
[fix][broker] namespace not found will cause request timeout (apache#…
e865a42 
…18512)

(cherry picked from commit 7b06489)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tjiuming tjiuming tjiuming approved these changes

@liangyepianzhou liangyepianzhou liangyepianzhou approved these changes

@nicoloboschi nicoloboschi nicoloboschi approved these changes

@RobertIndie RobertIndie RobertIndie approved these changes

@nodece nodece nodece approved these changes

@lifepuzzlefun lifepuzzlefun lifepuzzlefun approved these changes

@eolivelli eolivelli eolivelli approved these changes

Assignees

@labuladong labuladong

Labels
area/broker cherry-picked/branch-2.10 cherry-picked/branch-2.11 doc-not-needed Your PR changes do not impact docs ready-to-test release/2.10.6 release/2.11.3
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.

[Bug] bin/pulsar-admin topics subscriptions freeze if namespace not exists