[improve][misc] Return failed future in async method instead of throw exception directly

[Shawyeok]

Motivation

Throws an exception from a async method generally a bad practice, for example in ProducerBuilderImpl#createAsync, checkArgument may throws a runtime exception directly:

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerBuilderImpl.java

Lines 94 to 97 in 50b9a93

	public CompletableFuture<Producer<T>> createAsync() {
	// config validation
	checkArgument(!(conf.isBatchingEnabled() && conf.isChunkingEnabled()),
	"Batching and chunking of messages can't be enabled together");

If user call it in a async context, it may cause some unexpected behavior, here is a case:

CompletableFuture<xxx> future = new CompletableFuture<>();
xxx.whenComplete((v, ex) -> {
    xxx.createAsync().whenComplete((v, ex) -> {
        if (ex != null) {
            future.completeExceptionally(ex);
        } else {
            future.complete(v);
        }
    });
})

If createAsync() throws an exception from inside, the future will never get completed. This kind of problem is very hard to troubleshooting cause it could haven’t any log.

So I wrote a PMD rule to examine the entire Pulsar codebase (current master branch), aiming to identify similar kinds of problems. Here it is:

//MethodDeclaration[matches(@Name, "Async$")]
  [.//PrimaryPrefix/Name[matches(@Image, "(requireNonNull|checkNotNull|checkState|checkArgument)$")]]

Note: this rule may not be able to scan all similar code issues.

Modifications

Return a failed future instead of throw exception directly, for the given case above, it's will be change to:

public CompletableFuture<Producer<T>> createAsync() {
    // config validation
    if (conf.isBatchingEnabled() && conf.isChunkingEnabled()) {
        return FutureUtil.failedFuture(
                new IllegalArgumentException("Batching and chunking of messages can't be enabled together"));
    }

Verifying this change

• Make sure that the change passes the CI checks.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

• Dependencies (add or upgrade a dependency)
• The public API
• The schema
• The default values of configurations
• The threading model
• The binary protocol
• The REST endpoints
• The admin CLI options
• The metrics
• Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository: Shawyeok#9

[lhotari]

Thanks for the contribution.

This is a breaking change in the API since exceptions could also be considered as part of the API although they aren't explicitly stated in the Java interface files. We use the PIP process to track changes to public APIs in Pulsar.

The other benefit of a PIP is that there will be broader awareness of the change in our approach to handle method parameter validation in asynchronous methods. The PIP process will help ensure better consistently in the project in the future.

@Shawyeok please start a new PIP for this change.

[lhotari]

Blocking the changes since I think that we need a PIP for public API changes. I provided the rational in my previous comment.

[github-actions]

The pr had no activity for 30 days, mark with Stale label.