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.

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

[fix][broker] Ensure that PulsarService is ready for serving incoming requests #22977

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
Copy link
Member Author

lhotari commented Jun 26, 2024

@heesung-sn I pushed an attempt to make this solution work with ExtensibleLoadManagerImpl.

@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 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 424 commits behind head on master.

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

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

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

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

(cherry picked from commit 53df683)
(cherry picked from commit ec51420)
heesung-sn pushed a commit to heesung-sn/pulsar that referenced this pull request Jun 26, 2024
heesung-sn added a commit to heesung-sn/pulsar that referenced this pull request Jun 27, 2024
heesung-sn pushed a commit to heesung-sn/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)
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