[fix][authorization] Fix the return value of canConsumeAsync

[nodece]

Motivation

When an exception occurs in the canConsumeAsync method, we should throw that.

Documentation

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

[lhotari]

Is the logging just extra noise if the exception gets rethrown? Could we remove the .exceptionally completely? My assumption is that rethrowing could lead to the exception being logged more than 1 time.

I guess return null; is wrong and could be return false; if the desire is to just log and continue by returning false. The original code (before this PR) will fail with NullPointerException since the expected return value is Boolean.

[michaeljmarshall]

We already log the "error" multiple times. We log it in this class as a warning. We also have these logs for handling the result of this method:

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

Lines 888 to 902 in 9521872

	return cnx.getBrokerService().getAuthorizationService().canConsumeAsync(topicName, appId,
	cnx.getAuthenticationData(), subscription.getName())
	.handle((ok, e) -> {
	if (e != null) {
	log.warn("[{}] Get unexpected error while authorizing [{}] {}", appId,
	subscription.getTopicName(), e.getMessage(), e);
	}
	if (ok == null || !ok) {
	log.info("[{}] is not allowed to consume from topic [{}] anymore", appId,
	subscription.getTopicName());
	disconnect();
	}
	return null;
	});

After this change, we could have 3 lines and a stack trace. I think that is too many. I would prefer to simplify the code path and only log a single failure/error/rejection once.

[michaeljmarshall]

My preference is to remove the exceptionally block all together and to make sure that pulsar logs the errors accordingly. Since this AuthorizationProvider is pluggable, it simplifies the flow if implementations can rely on pulsar to log appropriately in my opinion.

Let me know what you think @nodece. Thanks!

[michaeljmarshall]

We already log the "error" multiple times. We log it in this class as a warning. We also have these logs for handling the result of this method:

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

Lines 888 to 902 in 9521872

	return cnx.getBrokerService().getAuthorizationService().canConsumeAsync(topicName, appId,
	cnx.getAuthenticationData(), subscription.getName())
	.handle((ok, e) -> {
	if (e != null) {
	log.warn("[{}] Get unexpected error while authorizing [{}] {}", appId,
	subscription.getTopicName(), e.getMessage(), e);
	}
	if (ok == null || !ok) {
	log.info("[{}] is not allowed to consume from topic [{}] anymore", appId,
	subscription.getTopicName());
	disconnect();
	}
	return null;
	});

After this change, we could have 3 lines and a stack trace. I think that is too many. I would prefer to simplify the code path and only log a single failure/error/rejection once.

[nodece]

I want to remove all exceptionally, then we can optimize the log info next PR. what do you think of that?

[michaeljmarshall]

LGTM, thanks @nodece!

[nodece]

/pulsarbot rerun-failure-checks

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 26.48%. Comparing base (39dd1cd) to head (2e95581).
⚠️ Report is 2889 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #19412      +/-   ##
============================================
- Coverage     32.36%   26.48%   -5.88%     
- Complexity     6355     6369      +14     
============================================
  Files          1648     1581      -67     
  Lines        123990   121828    -2162     
  Branches      13523    13304     -219     
============================================
- Hits          40129    32271    -7858     
- Misses        77958    84925    +6967     
+ Partials       5903     4632    -1271     

Flag	Coverage Δ
inttests	24.87% <ø> (+0.05%)	⬆️
systests	∅ <ø> (∅)
unittests	17.48% <ø> (-0.07%)	⬇️

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

Files with missing lines	Coverage Δ
...ker/authorization/PulsarAuthorizationProvider.java	8.01% <ø> (-27.28%)	⬇️

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