Skip to content

Conversation

@lhotari
Copy link
Member

@lhotari lhotari commented Jun 25, 2024

Fixes #22975

Motivation

Pulsar broker will start serving requests while the broker is starting. This can cause issues and bugs which are hard to reproduce. Serving requests should be delayed until the broker is ready.
The brokerId bug #22975 is just one example of problems that this PR will address.

Modifications

  • start Pulsar binary protocol connections with autoread set to false so that request handling can be postponed
  • enable autoread after PulsarService has been started
    • add solution to PulsarService for running a function when the service has been started
  • add a filter to REST implementation which blocks until PulsarService is started
    • add solution to PulsarService to wait until the service has been started

Documentation

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

@dao-jun
Copy link
Member

dao-jun commented Jun 25, 2024

Is this possible for us to start WebService/WebSocketService/BrokerService/ProtocolHander after the other Pulsar Services start?

@lhotari
Copy link
Member Author

lhotari commented Jun 25, 2024

Is this possible for us to start WebService/WebSocketService/BrokerService/ProtocolHander after the other Pulsar Services start?

Yes, it makes sense to cover all cases in a way so that requests aren't handled before PulsarService has been started.

@lhotari lhotari marked this pull request as draft June 25, 2024 15:03
@lhotari lhotari force-pushed the lh-ensure-PulsarService-is-started-before-serving-requests branch from 49ae27c to 1238a4f Compare June 26, 2024 06:14
@lhotari lhotari marked this pull request as ready for review June 26, 2024 06:15
@lhotari lhotari changed the title [fix][broker] Ensure that PulsarService is started before serving requests [fix][broker] Ensure that PulsarService is ready before serving requests Jun 26, 2024
@lhotari
Copy link
Member Author

lhotari commented Jun 26, 2024

Is this possible for us to start WebService/WebSocketService/BrokerService/ProtocolHander after the other Pulsar Services start?

Yes, it makes sense to cover all cases in a way so that requests aren't handled before PulsarService has been started.

@dao-jun

I renamed the concept to mean that the broker is ready to serve request instead of being fully started. I believe that this covers what is really needed and the original intention of this PR.

I checked the current solution and this is sufficiently covered already for the Pulsar broker with various configurations. There's no need to separately handle websockets since websocket servlets get added to the same Jetty server which already contains the filter to wait until PulsarService is ready for incoming requests.

ProtocolHandlers don't need any special handling since they get started as the last step in PulsarService.

@lhotari lhotari force-pushed the lh-ensure-PulsarService-is-started-before-serving-requests branch from 1238a4f to 16c69f1 Compare June 26, 2024 06:28
@lhotari lhotari changed the title [fix][broker] Ensure that PulsarService is ready before serving requests [fix][broker] Ensure that PulsarService is ready for serving incoming requests Jun 26, 2024
@lhotari lhotari force-pushed the lh-ensure-PulsarService-is-started-before-serving-requests branch from 16c69f1 to 09a46e4 Compare June 26, 2024 06:35
@lhotari lhotari force-pushed the lh-ensure-PulsarService-is-started-before-serving-requests branch 2 times, most recently from 14c17a0 to 3ab5526 Compare June 26, 2024 11:47
@lhotari lhotari force-pushed the lh-ensure-PulsarService-is-started-before-serving-requests branch from 3ab5526 to fc593e7 Compare June 26, 2024 11:48
@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2024

Codecov Report

Attention: Patch coverage is 77.38095% with 19 lines in your changes missing coverage. Please review.

Project coverage is 73.39%. Comparing base (bbc6224) to head (fc593e7).
Report is 1164 commits behind head on master.

Files with missing lines Patch % Lines
...dbalance/extensions/ExtensibleLoadManagerImpl.java 74.50% 13 Missing ⚠️
.../java/org/apache/pulsar/broker/web/WebService.java 62.50% 6 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22977      +/-   ##
============================================
- Coverage     73.57%   73.39%   -0.18%     
- Complexity    32624    32915     +291     
============================================
  Files          1877     1903      +26     
  Lines        139502   142711    +3209     
  Branches      15299    15571     +272     
============================================
+ Hits         102638   104749    +2111     
- Misses        28908    29941    +1033     
- Partials       7956     8021      +65     
Flag Coverage Δ
inttests 27.73% <72.61%> (+3.14%) ⬆️
systests 24.73% <27.38%> (+0.40%) ⬆️
unittests 72.43% <77.38%> (-0.42%) ⬇️

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

Files with missing lines Coverage Δ
...n/java/org/apache/pulsar/broker/PulsarService.java 83.10% <100.00%> (+0.73%) ⬆️
...ache/pulsar/broker/namespace/NamespaceService.java 73.21% <100.00%> (+0.97%) ⬆️
...ulsar/broker/service/PulsarChannelInitializer.java 95.08% <100.00%> (+0.08%) ⬆️
...va/org/apache/pulsar/broker/service/ServerCnx.java 72.18% <100.00%> (+0.03%) ⬆️
.../java/org/apache/pulsar/broker/web/WebService.java 90.78% <62.50%> (-3.36%) ⬇️
...dbalance/extensions/ExtensibleLoadManagerImpl.java 80.52% <74.50%> (+0.44%) ⬆️

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

@lhotari lhotari merged commit 53df683 into apache:master Jun 26, 2024
heesung-sohn pushed a commit to heesung-sohn/pulsar that referenced this pull request Jun 26, 2024
heesung-sohn pushed a commit to heesung-sohn/pulsar that referenced this pull request Jun 26, 2024
heesung-sohn pushed a commit to heesung-sohn/pulsar that referenced this pull request Jun 26, 2024
… requests (apache#22977)

(cherry picked from commit 53df683)
(cherry picked from commit ec51420)
heesung-sohn pushed a commit to heesung-sohn/pulsar that referenced this pull request Jun 26, 2024
heesung-sohn pushed a commit to heesung-sohn/pulsar that referenced this pull request Jun 26, 2024
heesung-sohn pushed a commit to heesung-sohn/pulsar that referenced this pull request Jun 26, 2024
heesung-sohn pushed a commit to heesung-sohn/pulsar that referenced this pull request Jun 26, 2024
… requests (apache#22977)

(cherry picked from commit 53df683)
(cherry picked from commit ec51420)
heesung-sohn pushed a commit to heesung-sohn/pulsar that referenced this pull request Jun 26, 2024
… requests (apache#22977)

(cherry picked from commit 53df683)
(cherry picked from commit ec51420)
heesung-sohn pushed a commit to heesung-sohn/pulsar that referenced this pull request Jun 26, 2024
heesung-sohn pushed a commit to heesung-sohn/pulsar that referenced this pull request Jun 27, 2024
heesung-sohn pushed a commit to heesung-sohn/pulsar that referenced this pull request Jun 27, 2024
… requests (apache#22977)

(cherry picked from commit 53df683)
(cherry picked from commit ec51420)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 27, 2024
… requests (apache#22977)

(cherry picked from commit 53df683)
(cherry picked from commit ec51420)
(cherry picked from commit 1a7eb54)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jul 1, 2024
… requests (apache#22977)

(cherry picked from commit 53df683)
(cherry picked from commit ec51420)
(cherry picked from commit 1a7eb54)
hanmz pushed a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug][broker] BrokerId npe when broker restart

8 participants