codelipenghui commented on code in PR #25289:
URL: https://github.com/apache/pulsar/pull/25289#discussion_r2891568596
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/netty/ChannelFutures.java:
##########
@@ -41,7 +41,9 @@ private ChannelFutures() {
* and completes exceptionally if the channelFuture completes with
a {@link Throwable}
*/
public static CompletableFuture<Channel> toCompletableFuture(ChannelFuture
channelFuture) {
- Objects.requireNonNull(channelFuture, "channelFuture cannot be null");
+ if (channelFuture == null) {
Review Comment:
This will break the existing test
`ChannelFuturesTest.toCompletableFuture_shouldRequireNonNullArgument` which
uses `@Test(expectedExceptions = NullPointerException.class)` and expects a
synchronous throw. After this change the method returns a failed future
instead. The test needs to be updated to verify the future completes
exceptionally with `NullPointerException`.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##########
@@ -1145,8 +1145,12 @@ public NamespaceBundleSplitAlgorithm
getNamespaceBundleSplitAlgorithmByName(Stri
*/
public CompletableFuture<Void>
updateNamespaceBundlesForPolicies(NamespaceName nsname,
NamespaceBundles nsBundles) {
- Objects.requireNonNull(nsname);
- Objects.requireNonNull(nsBundles);
+ if (nsname == null) {
+ return FutureUtil.failedFuture(new NullPointerException("Excepted
NamespaceName should not be null "));
Review Comment:
Typo: `"Excepted"` should be `"Expected"`. Also has a trailing space before
the closing quote. The sibling method `updateNamespaceBundles` below correctly
uses `"Expected NamespaceName should not be null"`.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/Bucket.java:
##########
@@ -158,7 +157,9 @@ CompletableFuture<Long> asyncSaveBucketSnapshot(
}
private CompletableFuture<Void> putBucketKeyId(String bucketKey, Long
bucketId) {
- Objects.requireNonNull(bucketId);
+ if (bucketId == null) {
+ return FutureUtil.failedFuture(new NullPointerException());
Review Comment:
nit: `new NullPointerException()` has no message. Consider `new
NullPointerException("bucketId")` to aid debugging.
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/FutureUtil.java:
##########
@@ -231,7 +230,9 @@ public static <T> Sequencer<T> create() {
* @throws NullPointerException NPE when param is null
Review Comment:
This `@throws NullPointerException` Javadoc is now inaccurate — the method
returns a failed future instead of throwing. Same issue for `composeAsync`
below.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/LoadManager.java:
##########
@@ -70,11 +70,11 @@ default boolean started() {
default CompletableFuture<Optional<LookupResult>> findBrokerServiceUrl(
Optional<ServiceUnitId> topic, ServiceUnitId bundle, LookupOptions
options) {
- throw new UnsupportedOperationException();
+ return CompletableFuture.failedFuture(new
UnsupportedOperationException());
Review Comment:
nit: This uses `CompletableFuture.failedFuture()` while most other changes
in this PR use `FutureUtil.failedFuture()`. The usage is inconsistent across
the PR — consider standardizing on one approach.
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/FutureUtil.java:
##########
@@ -287,8 +288,9 @@ public static <T> CompletableFuture<T>
addTimeoutHandling(CompletableFuture<T> f
*/
public static <T> @NonNull CompletableFuture<T>
composeAsync(Supplier<CompletableFuture<T>> futureSupplier,
Executor
executor) {
- Objects.requireNonNull(futureSupplier);
- Objects.requireNonNull(executor);
+ if (futureSupplier == null || executor == null) {
Review Comment:
nit: Merging two separate null checks into one loses diagnostic specificity
— you can't tell which parameter was null from the empty NPE. Consider keeping
separate checks with messages.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
