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

[Technoboy-]

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

[codecov-commenter]

Codecov Report

Merging #20710 (dd355b1) into master (d85b995) will increase coverage by 2.13%.
The diff coverage is 16.21%.

@@             Coverage Diff              @@
##             master   #20710      +/-   ##
============================================
+ Coverage     34.70%   36.83%   +2.13%     
- Complexity    11994    12181     +187     
============================================
  Files          1694     1691       -3     
  Lines        129459   130030     +571     
  Branches      14132    14229      +97     
============================================
+ Hits          44925    47900    +2975     
+ Misses        78571    75844    -2727     
- Partials       5963     6286     +323     

Flag	Coverage Δ
inttests	24.21% <10.81%> (?)
systests	24.99% <13.51%> (+<0.01%)	⬆️
unittests	32.01% <31.57%> (+0.13%)	⬆️

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

Impacted Files	Coverage Δ
...org/apache/pulsar/broker/ServiceConfiguration.java	98.04% <ø> (ø)
...in/java/org/apache/pulsar/PulsarBrokerStarter.java	28.14% <ø> (-3.00%)	⬇️
.../org/apache/pulsar/PulsarClusterMetadataSetup.java	16.37% <ø> (+16.37%)	⬆️
...g/apache/pulsar/PulsarClusterMetadataTeardown.java	71.71% <ø> (+71.71%)	⬆️
...org/apache/pulsar/PulsarInitialNamespaceSetup.java	0.00% <ø> (ø)
...ava/org/apache/pulsar/PulsarStandaloneStarter.java	29.23% <ø> (ø)
...sar/PulsarTransactionCoordinatorMetadataSetup.java	50.00% <ø> (+50.00%)	⬆️
...n/java/org/apache/pulsar/PulsarVersionStarter.java	0.00% <ø> (ø)
...he/pulsar/broker/intercept/BrokerInterceptors.java	6.95% <ø> (-2.37%)	⬇️
...pache/pulsar/broker/tools/GenerateDocsCommand.java	0.00% <ø> (ø)
... and 13 more

... and 185 files with indirect coverage changes

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

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

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.