Skip to content

[fix][broker] Skip loading broker interceptor when disableBrokerInterceptors is true#20422

Merged
nodece merged 1 commit into
apache:masterfrom
nodece:skip-broker-interceptor
May 30, 2023
Merged

[fix][broker] Skip loading broker interceptor when disableBrokerInterceptors is true#20422
nodece merged 1 commit into
apache:masterfrom
nodece:skip-broker-interceptor

Conversation

@nodece

@nodece nodece commented May 29, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Motivation

When disableBrokerInterceptors is true, we should skip loading the broker interceptor to avoid initializing it.

This configuration is true by default, whether enabled or not, it will not affect the broker feature.

By the way, the web service and broker service should keep the same behavior:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java

Lines 216 to 224 in b7f7e04

boolean brokerInterceptorEnabled =
pulsarService.getBrokerInterceptor() != null && !config.isDisableBrokerInterceptors();
if (brokerInterceptorEnabled) {
ExceptionHandler handler = new ExceptionHandler();
// Enable PreInterceptFilter only when interceptors are enabled
filterHolders.add(
new FilterHolder(new PreInterceptFilter(pulsarService.getBrokerInterceptor(), handler)));
filterHolders.add(new FilterHolder(new ProcessHandlerFilter(pulsarService.getBrokerInterceptor())));
}

Modifications

  • Check whether disableBrokerInterceptors value, and then load the broker interceptor
  • Update disableBrokerInterceptors description

Documentation

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

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label May 29, 2023
@nodece nodece self-assigned this May 29, 2023
@nodece

nodece commented May 29, 2023

Copy link
Copy Markdown
Member Author

/pulsarbot rerun-failure-checks

@codecov-commenter

codecov-commenter commented May 29, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 36.83%. Comparing base (aa3bfcd) to head (d4eaa21).
⚠️ Report is 2488 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #20422       +/-   ##
=============================================
- Coverage     72.90%   36.83%   -36.08%     
+ Complexity    31864    12053    -19811     
=============================================
  Files          1864     1688      -176     
  Lines        138416   128884     -9532     
  Branches      15188    14022     -1166     
=============================================
- Hits         100919    47477    -53442     
- Misses        29477    75152    +45675     
+ Partials       8020     6255     -1765
Flag Coverage Δ
inttests 24.17% <66.66%> (+<0.01%) ⬆️
systests 25.00% <66.66%> (-0.06%) ⬇️
unittests 31.97% <100.00%> (-40.21%) ⬇️

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

Files with missing lines Coverage Δ
...org/apache/pulsar/broker/ServiceConfiguration.java 98.02% <ø> (-1.36%) ⬇️
...he/pulsar/broker/intercept/BrokerInterceptors.java 9.32% <100.00%> (-37.64%) ⬇️

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

tisonkun
tisonkun reviewed May 29, 2023
View reviewed changes

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

@nodece what is the real-world affect without this fix? Will the broker crash or thow exception?

This can be semantically correct while if we have a real world problem, we may add a test case to prevent regressions.

@nodece

nodece commented May 29, 2023

Copy link
Copy Markdown
Member Author

@tisonkun

@nodece what is the real-world affect without this fix?

Without this patch, the broker always inits the broker interceptor and calls it.

Will the broker crash or thow exception?

The broker works fine.

This can be semantically correct while if we have a real world problem, we may add a test case to prevent regressions.

Perhaps I should add a test to check this call.

@nodece
[fix][broker] Skip loading broker interceptor when disableBrokerInter…
d4eaa21 
…ceptors is true

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@nodece nodece changed the title [fix][broker] Skip loading broker interceptor when disabled [fix][broker] Skip loading broker interceptor when disableBrokerInterceptors is true May 29, 2023
@nodece nodece force-pushed the skip-broker-interceptor branch from 7051026 to d4eaa21 Compare May 29, 2023 08:55
@nodece nodece requested a review from tisonkun May 29, 2023 08:56
@nodece nodece merged commit 639c460 into apache:master May 30, 2023
@Technoboy- Technoboy- added this to the 3.1.0 milestone May 30, 2023
Technoboy- pushed a commit that referenced this pull request May 30, 2023
@nodece @Technoboy-
[fix][broker] Skip loading broker interceptor when disableBrokerInter…
61e8463 
…ceptors is true (#20422)

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
poorbarcode pushed a commit that referenced this pull request May 30, 2023
@nodece @poorbarcode
[fix][broker] Skip loading broker interceptor when disableBrokerInter…
1ce9cca 
…ceptors is true (#20422)

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
(cherry picked from commit 639c460)
Technoboy- added a commit to Technoboy-/pulsar that referenced this pull request Jul 4, 2023
@Technoboy-
Revert "[fix][broker] Skip loading broker interceptor when disableBro…
dd355b1 
…kerInterceptors is true (apache#20422)"

This reverts commit 639c460.
This was referenced Jul 4, 2023
Closed
Closed
Merged
Technoboy- added a commit that referenced this pull request Jul 5, 2023
@Technoboy-
[fix][broker] Revert "Skip loading broker interceptor when disableBro…
efea294 
…kerInterceptors is true #20422" (#20710)
Technoboy- added a commit that referenced this pull request Jul 5, 2023
@Technoboy-
[fix][broker] Revert "Skip loading broker interceptor when disableBro…
25d3832 
…kerInterceptors is true #20422" (#20710)
Technoboy- added a commit that referenced this pull request Jul 5, 2023
@Technoboy-
[fix][broker] Revert "Skip loading broker interceptor when disableBro…
b5a29e7 
…kerInterceptors is true #20422" (#20710)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tisonkun tisonkun tisonkun approved these changes

@poorbarcode poorbarcode poorbarcode approved these changes

@lhotari lhotari Awaiting requested review from lhotari

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@coderzc coderzc Awaiting requested review from coderzc

@congbobo184 congbobo184 Awaiting requested review from congbobo184

@mattisonchao mattisonchao Awaiting requested review from mattisonchao

Assignees

@nodece nodece

Labels

area/broker cherry-picked/branch-2.11 cherry-picked/branch-3.0 doc-not-needed Your PR changes do not impact docs ready-to-test release/2.11.2 release/3.0.1

Projects

None yet

Milestone

3.1.0

Development

Successfully merging this pull request may close these issues.

5 participants

@nodece @codecov-commenter @tisonkun @poorbarcode @Technoboy-