apurtell commented on code in PR #4634:
URL: https://github.com/apache/hbase/pull/4634#discussion_r929364009
##########
hbase-asyncfs/src/main/java/org/apache/hadoop/hbase/io/asyncfs/FanOutOneBlockAsyncDFSOutput.java:
##########
@@ -252,7 +256,7 @@ private synchronized void failed(Channel channel,
Supplier<Throwable> errorSuppl
// disable further write, and fail all pending ack.
state = State.BROKEN;
failWaitingAckQueue(channel, errorSupplier);
- datanodeInfoMap.keySet().forEach(ChannelOutboundInvoker::close);
+ datanodeInfoMap.keySet().forEach(NettyFutureUtils::safeClose);
Review Comment:
I like the approach here where new `NettyFutureUtils` encapsulates the
await/cleanup details.
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/FutureUtils.java:
##########
@@ -93,6 +93,17 @@ public static <T> void addListener(CompletableFuture<T>
future,
}, executor);
}
+ /**
+ * Log the error if the future indicates any failure.
+ */
+ public static void consume(CompletableFuture<?> future) {
+ addListener(future, (r, e) -> {
+ if (e != null) {
+ LOG.warn("Async operation failes", e);
Review Comment:
typo: "failes" should be "fails"
##########
hbase-asyncfs/src/main/java/org/apache/hadoop/hbase/io/asyncfs/FanOutOneBlockAsyncDFSOutputHelper.java:
##########
@@ -360,7 +363,7 @@ private static void initialize(Configuration conf, Channel
channel, DatanodeInfo
throws IOException {
Promise<Void> saslPromise = channel.eventLoop().newPromise();
trySaslNegotiate(conf, channel, dnInfo, timeoutMs, client, accessToken,
saslPromise);
- saslPromise.addListener(new FutureListener<Void>() {
+ addListener(saslPromise, new FutureListener<Void>() {
Review Comment:
"Promoting" the listener makes sense.
##########
hbase-examples/src/main/java/org/apache/hadoop/hbase/client/example/HttpProxyExample.java:
##########
@@ -239,6 +240,7 @@ protected void initChannel(Channel ch) throws Exception {
}).bind(port).syncUninterruptibly().channel();
}
+ @SuppressWarnings("FutureReturnValueIgnored")
Review Comment:
This is example code so ideally we show how to properly manage futures here.
In this case just adding a comment at the top of the file should be good, to
explain the suppressions.
/*
* Future class methods will all return a new {@link Future}, so you
always have one future
* that will not been checked, so we need to suppress error-prone
"FutureReturnValueIgnored"
* warnings
*/
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/NettyFutureUtils.java:
##########
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.util;
+
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.io.netty.channel.ChannelOutboundInvoker;
+import org.apache.hbase.thirdparty.io.netty.util.concurrent.Future;
+import
org.apache.hbase.thirdparty.io.netty.util.concurrent.GenericFutureListener;
+
+/**
+ * Helper class for processing netty futures.
+ */
[email protected]
+public final class NettyFutureUtils {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(NettyFutureUtils.class);
+
+ private NettyFutureUtils() {
+ }
+
+ /**
+ * This is method is used when you just want to add a listener to the given
netty future. Ignoring
+ * the return value of a Future is considered as a bad practice as it may
suppress exceptions
+ * thrown from the code that completes the future, and this method will
catch all the exception
+ * thrown from the {@code listener} to catch possible code bugs.
+ * <p/>
+ * And the error phone check will always report FutureReturnValueIgnored
because every method in
Review Comment:
Sounds good.
--
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]
