[fix][broker] Revert "Skip loading broker interceptor when disableBrokerInterceptors is true #20422" by Technoboy- · Pull Request #20710 · apache/pulsar

Technoboy- · 2023-07-04T05:26:27Z

Motivation

After merging #20422 , the 2.11 may encounter NPE when config :

brokerInterceptors = xxx-interceptor
disableBrokerInterceptors = true

2023-07-03T10:26:00,507+0000 [main] INFO  org.apache.pulsar.broker.intercept.BrokerInterceptors - Skip loading the broker interceptors when disableBrokerInterceptors is true

See code of branch-2.11

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

Lines 38 to 53 in dc25810

    
           public ProcessHandlerFilter(PulsarService pulsar) { 
        
               this.interceptor = pulsar.getBrokerInterceptor(); 
        
               this.interceptorEnabled = !pulsar.getConfig().getBrokerInterceptors().isEmpty(); 
        
           } 
        
           @Override 
        
           public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) 
        
                   throws IOException, ServletException { 
        
               if (interceptorEnabled 
        
                       && !StringUtils.containsIgnoreCase(request.getContentType(), MediaType.MULTIPART_FORM_DATA) 
        
                       && !StringUtils.containsIgnoreCase(request.getContentType(), MediaType.APPLICATION_OCTET_STREAM)) { 
        
                   interceptor.onFilter(request, response, chain); 
        
               } else { 
        
                   chain.doFilter(request, response); 
        
               } 
        
           }

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/intercept/BrokerInterceptors.java

Lines 60 to 64 in dc25810

    
           public static BrokerInterceptor load(ServiceConfiguration conf) throws IOException { 
        
               if (conf.isDisableBrokerInterceptors()) { 
        
                   log.info("Skip loading the broker interceptors when disableBrokerInterceptors is true"); 
        
                   return null; 
        
               }

The master branch has fixed the NPE by #19376. it's hard to cherry-pick that to branch-2.11.
So in order to keep the same logic, we need to revert #20422 first and then discuss if we should keep disableBrokerInterceptors in the mail list.

Documentation

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

…kerInterceptors is true (apache#20422)" This reverts commit 639c460.

codecov-commenter · 2023-07-04T11:50:48Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.99%. Comparing base (d85b995) to head (dd355b1).
⚠️ Report is 1855 commits behind head on master.

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #20710       +/-   ##
=============================================
+ Coverage     34.70%   72.99%   +38.29%     
- Complexity    11994    32012    +20018     
=============================================
  Files          1694     1868      +174     
  Lines        129459   139619    +10160     
  Branches      14132    15400     +1268     
=============================================
+ Hits          44925   101911    +56986     
+ Misses        78571    29631    -48940     
- Partials       5963     8077     +2114

Flag	Coverage Δ
inttests	`24.29% <ø> (?)`
systests	`24.99% <ø> (+<0.01%)`	⬆️
unittests	`72.18% <ø> (+40.30%)`	⬆️

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	`99.38% <ø> (+1.33%)`	⬆️
...he/pulsar/broker/intercept/BrokerInterceptors.java	`46.95% <ø> (+37.63%)`	⬆️

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

nodece

Why not fix the branch-2.11? I think this is the correct behavior that skips loading broker interceptor when disableBrokerInterceptors is true.

Technoboy- · 2023-07-05T00:40:42Z

Why not fix the branch-2.11? I think this is the correct behavior that skips loading broker interceptor when disableBrokerInterceptors is true.

If fix 2.11, both master and 2.11 use disableBrokerInterceptors for judgment, but other branches do not

Technoboy- · 2023-07-05T00:56:37Z

Why not fix the branch-2.11? I think this is the correct behavior that skips loading broker interceptor when disableBrokerInterceptors is true.

We need to discuss this disableBrokerInterceptors , as when it was introduced by #8157 (pulsar-2.7), it uses for test only.

codelipenghui

I support reverting it first and then finding a solution to fix the bug. Just to avoid any potential break changes introduced to the subsequent releases.

…kerInterceptors is true #20422" (#20710)

Revert "[fix][broker] Skip loading broker interceptor when disableBro…

dd355b1

…kerInterceptors is true (apache#20422)" This reverts commit 639c460.

Technoboy- self-assigned this Jul 4, 2023

Technoboy- added this to the 3.1.0 milestone Jul 4, 2023

Technoboy- added doc-not-needed Your PR changes do not impact docs ready-to-test release/2.11.2 release/3.0.2 labels Jul 4, 2023

github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Jul 4, 2023

Technoboy- closed this Jul 4, 2023

Technoboy- reopened this Jul 4, 2023

tisonkun requested a review from nodece July 4, 2023 05:38

Technoboy- removed the release/2.11.2 label Jul 4, 2023

Technoboy- closed this Jul 4, 2023

Technoboy- reopened this Jul 4, 2023

Technoboy- added the release/2.11.2 label Jul 4, 2023

mattisonchao approved these changes Jul 4, 2023

View reviewed changes

nodece reviewed Jul 4, 2023

View reviewed changes

codelipenghui approved these changes Jul 5, 2023

View reviewed changes

poorbarcode approved these changes Jul 5, 2023

View reviewed changes

Technoboy- merged commit efea294 into apache:master Jul 5, 2023

Technoboy- added a commit that referenced this pull request Jul 5, 2023

[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

[fix][broker] Revert "Skip loading broker interceptor when disableBro…

b5a29e7

…kerInterceptors is true #20422" (#20710)

Technoboy- added cherry-picked/branch-2.11 cherry-picked/branch-3.0 labels Jul 5, 2023

RobertIndie added release/3.0.1 and removed release/3.0.2 labels Jul 13, 2023

RobertIndie added the area/broker label Aug 7, 2023

Technoboy- deleted the revert-20422 branch November 11, 2023 07:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

[fix][broker] Revert "Skip loading broker interceptor when disableBrokerInterceptors is true #20422"#20710
Technoboy- merged 1 commit into
apache:masterfrom
Technoboy-:revert-20422

Technoboy- commented Jul 4, 2023 •

edited

Loading

Uh oh!

codecov-commenter commented Jul 4, 2023 •

edited

Loading

Uh oh!

nodece left a comment

Uh oh!

Technoboy- commented Jul 5, 2023

Uh oh!

Technoboy- commented Jul 5, 2023 •

edited

Loading

Uh oh!

codelipenghui left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

	public ProcessHandlerFilter(PulsarService pulsar) {
	this.interceptor = pulsar.getBrokerInterceptor();
	this.interceptorEnabled = !pulsar.getConfig().getBrokerInterceptors().isEmpty();
	}

	@Override
	public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
	throws IOException, ServletException {
	if (interceptorEnabled
	&& !StringUtils.containsIgnoreCase(request.getContentType(), MediaType.MULTIPART_FORM_DATA)
	&& !StringUtils.containsIgnoreCase(request.getContentType(), MediaType.APPLICATION_OCTET_STREAM)) {
	interceptor.onFilter(request, response, chain);
	} else {
	chain.doFilter(request, response);
	}
	}

	public static BrokerInterceptor load(ServiceConfiguration conf) throws IOException {
	if (conf.isDisableBrokerInterceptors()) {
	log.info("Skip loading the broker interceptors when disableBrokerInterceptors is true");
	return null;
	}

Conversation

Technoboy- commented Jul 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Documentation

Uh oh!

codecov-commenter commented Jul 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nodece left a comment

Choose a reason for hiding this comment

Uh oh!

Technoboy- commented Jul 5, 2023

Uh oh!

Technoboy- commented Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codelipenghui left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Technoboy- commented Jul 4, 2023 •

edited

Loading

codecov-commenter commented Jul 4, 2023 •

edited

Loading

Technoboy- commented Jul 5, 2023 •

edited

Loading