[improve][broker] Omit making a copy of CommandAck when there are no broker interceptors

[lhotari]

Motivation

While fixing a bug, #18987 introduces a performance regression to handleAck method. The copy of the CommandAck instance should only be done if there's a broker interceptor in use.

Modifications

• Use a null value to indicate that the broker interceptor is missing
• Make changes to handle the null broker interceptor
• Omit making a copy of CommandAck in handleAck method when there are no broker interceptors

Documentation

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

Matching PR in forked repository

PR in forked repository: lhotari#114

[nicoloboschi]

There are some spots where the NPE is not guarded, for example here:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Producer.java

Lines 274 to 275 in 37ea179

	this.cnx.getBrokerService().getInterceptor()
	.onMessagePublish(this, headersAndPayload, messagePublishContext);

[lhotari]

There are some spots where the NPE is not guarded, for example here:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Producer.java

Lines 274 to 275 in 37ea179

	this.cnx.getBrokerService().getInterceptor()
	.onMessagePublish(this, headersAndPayload, messagePublishContext);

Thanks @nicoloboschi, I pushed a fix. PTAL

[lhotari]

/pulsarbot rerun-failure-checks

[codecov-commenter]

Codecov Report

Merging #18997 (1e578c9) into master (22866bd) will decrease coverage by 4.46%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master   #18997      +/-   ##
============================================
- Coverage     45.92%   41.46%   -4.47%     
+ Complexity    10104     2399    -7705     
============================================
  Files           680      235     -445     
  Lines         66758    16652   -50106     
  Branches       7147     1812    -5335     
============================================
- Hits          30660     6904   -23756     
+ Misses        32680     8992   -23688     
+ Partials       3418      756    -2662     

Flag	Coverage Δ
unittests	41.46% <ø> (-4.47%)	⬇️

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

Impacted Files	Coverage Δ
...g/apache/pulsar/client/impl/ConnectionHandler.java	50.00% <0.00%> (-5.32%)	⬇️
...apache/pulsar/client/impl/AutoClusterFailover.java	69.44% <0.00%> (-0.56%)	⬇️
...n/java/org/apache/pulsar/broker/PulsarService.java
...che/pulsar/broker/intercept/BrokerInterceptor.java
...he/pulsar/broker/intercept/BrokerInterceptors.java
...ava/org/apache/pulsar/broker/service/Producer.java
...pulsar/broker/service/PulsarCommandSenderImpl.java
...va/org/apache/pulsar/broker/service/ServerCnx.java
...g/apache/pulsar/broker/web/PreInterceptFilter.java
...sar/broker/service/plugin/EntryFilterProvider.java
... and 494 more

review comments addressed

[nicoloboschi]

LGTM

[michaeljmarshall]

LGTM, thanks for catching the regression in my earlier PR.

[michaeljmarshall]

I cherry-picked to all of the relevant branches. My only concern is that we might end up causing issues for any custom code that runs in the broker. Even though this might break broker extensions, I think it is relevant to cherry pick this change. Otherwise, we're subject to significant performance regressions in most implementations (assuming that most users do not have broker interceptors). Maybe it's sufficient to put a note in the release notes?

[michaeljmarshall]

Release note: remove BrokerInterceptorDisabled in favor of a null value indicating that the interceptor is disabled.