This is an automated email from the ASF dual-hosted git repository.
szetszwo pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/ratis.git
The following commit(s) were added to refs/heads/master by this push:
new 70336088d RATIS-1946. PeerProxyMap: Stack trace may not be printed.
(#976)
70336088d is described below
commit 70336088d3a5bb488b6b91330ebda20ff1448e90
Author: Tsz-Wo Nicholas Sze <[email protected]>
AuthorDate: Fri Dec 1 09:23:02 2023 -0800
RATIS-1946. PeerProxyMap: Stack trace may not be printed. (#976)
---
.../java/org/apache/ratis/util/PeerProxyMap.java | 28 ++++++++++---
.../java/org/apache/ratis/util/StringUtils.java | 17 ++++++++
.../src/test/java/org/apache/ratis/BaseTest.java | 5 ++-
.../apache/ratis/server/impl/LeaderElection.java | 6 +--
.../ratis/server/leader/LogAppenderDefault.java | 4 +-
.../server/storage/RaftStorageDirectoryImpl.java | 12 ++----
.../server/simulation/SimulatedServerRpc.java | 3 +-
.../org/apache/ratis/util/TestPeerProxyMap.java | 49 ++++++++++++++++++++++
8 files changed, 101 insertions(+), 23 deletions(-)
diff --git a/ratis-common/src/main/java/org/apache/ratis/util/PeerProxyMap.java
b/ratis-common/src/main/java/org/apache/ratis/util/PeerProxyMap.java
index c5a08f56a..105ecbfb4 100644
--- a/ratis-common/src/main/java/org/apache/ratis/util/PeerProxyMap.java
+++ b/ratis-common/src/main/java/org/apache/ratis/util/PeerProxyMap.java
@@ -27,7 +27,11 @@ import org.slf4j.LoggerFactory;
import java.io.Closeable;
import java.io.IOException;
+import java.util.ArrayList;
import java.util.Collection;
+import java.util.Collections;
+import java.util.Iterator;
+import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
@@ -79,7 +83,7 @@ public class PeerProxyMap<PROXY extends Closeable> implements
RaftPeer.Add, Clos
@Override
public String toString() {
- return peer.toString();
+ return peer.getId().toString();
}
}
@@ -142,7 +146,8 @@ public class PeerProxyMap<PROXY extends Closeable>
implements RaftPeer.Add, Clos
}
}
// close proxy without holding the reset lock
- optional.ifPresent(proxy -> closeProxy(proxy, pp));
+ optional.map(proxy -> closeProxy(proxy, pp))
+ .ifPresent(e -> LOG.warn("Failed to close the previous proxy", e));
}
/** @return true if the given throwable is handled; otherwise, the call is a
no-op, return false. */
@@ -156,19 +161,30 @@ public class PeerProxyMap<PROXY extends Closeable>
implements RaftPeer.Add, Clos
@Override
public void close() {
+ final List<IOException> exceptions = Collections.synchronizedList(new
ArrayList<>());
ConcurrentUtils.parallelForEachAsync(peers.values(),
- pp -> pp.setNullProxyAndClose().ifPresent(proxy -> closeProxy(proxy,
pp)),
+ pp -> pp.setNullProxyAndClose().map(proxy -> closeProxy(proxy,
pp)).ifPresent(exceptions::add),
r -> new Thread(r).start()
).join();
+
+ final int size = exceptions.size();
+ if (size > 0) {
+ final Iterator<IOException> i = exceptions.iterator();
+ final IOException e = new IOException(name + ": Failed to close proxy
map", i.next());
+ for(; i.hasNext(); ) {
+ e.addSuppressed(i.next());
+ }
+ LOG.warn("{}: {} exception(s)", name, size, e);
+ }
}
- private void closeProxy(PROXY proxy, PeerAndProxy pp) {
+ private IOException closeProxy(PROXY proxy, PeerAndProxy pp) {
try {
LOG.debug("{}: Closing proxy for peer {}", name, pp);
proxy.close();
+ return null;
} catch (IOException e) {
- LOG.warn("{}: Failed to close proxy for peer {}, proxy class: {}",
- name, pp, proxy.getClass(), e);
+ return new IOException(name + ": Failed to close proxy for peer " + pp +
", " + proxy.getClass(), e);
}
}
}
\ No newline at end of file
diff --git a/ratis-common/src/main/java/org/apache/ratis/util/StringUtils.java
b/ratis-common/src/main/java/org/apache/ratis/util/StringUtils.java
index e1dae790c..0f3266e65 100644
--- a/ratis-common/src/main/java/org/apache/ratis/util/StringUtils.java
+++ b/ratis-common/src/main/java/org/apache/ratis/util/StringUtils.java
@@ -27,10 +27,12 @@ import java.io.StringWriter;
import java.nio.ByteBuffer;
import java.time.LocalDateTime;
import java.time.format.DateTimeFormatter;
+import java.util.Arrays;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.CompletableFuture;
+import java.util.function.Function;
import java.util.function.Supplier;
public final class StringUtils {
@@ -176,4 +178,19 @@ public final class StringUtils {
return "" + future.join();
}
}
+
+ public static <T> String array2String(T[] array, Function<T, String>
toStringMethod) {
+ if (array == null) {
+ return null;
+ } else if (array.length == 0) {
+ return "[]";
+ }
+
+ final StringBuilder b = new StringBuilder(128).append('[');
+ Arrays.stream(array).map(toStringMethod).forEach(s ->
b.append(s).append(", "));
+ final int length = b.length();
+ b.setCharAt(length - 2, ']');
+ b.setLength(length - 1);
+ return b.toString();
+ }
}
diff --git a/ratis-common/src/test/java/org/apache/ratis/BaseTest.java
b/ratis-common/src/test/java/org/apache/ratis/BaseTest.java
index b05387b7b..821369461 100644
--- a/ratis-common/src/test/java/org/apache/ratis/BaseTest.java
+++ b/ratis-common/src/test/java/org/apache/ratis/BaseTest.java
@@ -23,6 +23,7 @@ import org.apache.ratis.util.ExitUtils;
import org.apache.ratis.util.FileUtils;
import org.apache.ratis.util.JavaUtils;
import org.apache.ratis.util.Slf4jUtils;
+import org.apache.ratis.util.StringUtils;
import org.apache.ratis.util.TimeDuration;
import org.apache.ratis.util.function.CheckedRunnable;
import org.junit.After;
@@ -130,7 +131,9 @@ public abstract class BaseTest {
Class<? extends Throwable> expectedThrowableClass, Logger log,
Class<? extends Throwable>... expectedCauseClasses) {
if (log != null) {
- log.info("The test \"{}\" throws {}", description,
JavaUtils.getClassSimpleName(t.getClass()), t);
+ log.info("Expected the test \"{}\" to throw {} with cause(s) {}",
+ description, expectedThrowableClass.getSimpleName(),
+ StringUtils.array2String(expectedCauseClasses,
Class::getSimpleName));
}
Assert.assertEquals(expectedThrowableClass, t.getClass());
diff --git
a/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java
b/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java
index 43b778057..53e83d866 100644
---
a/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java
+++
b/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderElection.java
@@ -254,12 +254,10 @@ class LeaderElection implements Runnable {
}
final LifeCycle.State state = lifeCycle.getCurrentState();
if (state.isClosingOrClosed()) {
- LOG.info("{}: {} is safely ignored since this is already {}",
- this, JavaUtils.getClassSimpleName(e.getClass()), state, e);
+ LOG.info(this + ": since this is already " + state + ", safely ignore
" + e);
} else {
if (!server.getInfo().isAlive()) {
- LOG.info("{}: {} is safely ignored since the server is not alive:
{}",
- this, JavaUtils.getClassSimpleName(e.getClass()), server, e);
+ LOG.info(this + ": since the server is not alive, safely ignore " +
e);
} else {
LOG.error("{}: Failed, state={}", this, state, e);
}
diff --git
a/ratis-server/src/main/java/org/apache/ratis/server/leader/LogAppenderDefault.java
b/ratis-server/src/main/java/org/apache/ratis/server/leader/LogAppenderDefault.java
index 6f38f5009..21ef70d4d 100644
---
a/ratis-server/src/main/java/org/apache/ratis/server/leader/LogAppenderDefault.java
+++
b/ratis-server/src/main/java/org/apache/ratis/server/leader/LogAppenderDefault.java
@@ -87,7 +87,7 @@ class LogAppenderDefault extends LogAppenderBase {
} catch (IOException ioe) {
// TODO should have more detailed retry policy here.
if (retry++ % 10 == 0) { // to reduce the number of messages
- LOG.warn("{}: Failed to appendEntries (retry={}): {}", this,
retry++, ioe);
+ LOG.warn("{}: Failed to appendEntries (retry={})", this, retry, ioe);
}
handleException(ioe);
}
@@ -114,7 +114,7 @@ class LogAppenderDefault extends LogAppenderBase {
} catch (InterruptedIOException iioe) {
throw iioe;
} catch (Exception ioe) {
- LOG.warn("{}: Failed to installSnapshot {}: {}", this, snapshot, ioe);
+ LOG.warn("{}: Failed to installSnapshot {}", this, snapshot, ioe);
handleException(ioe);
return null;
}
diff --git
a/ratis-server/src/main/java/org/apache/ratis/server/storage/RaftStorageDirectoryImpl.java
b/ratis-server/src/main/java/org/apache/ratis/server/storage/RaftStorageDirectoryImpl.java
index 8cf7dd326..e7f69d1e2 100644
---
a/ratis-server/src/main/java/org/apache/ratis/server/storage/RaftStorageDirectoryImpl.java
+++
b/ratis-server/src/main/java/org/apache/ratis/server/storage/RaftStorageDirectoryImpl.java
@@ -157,10 +157,10 @@ class RaftStorageDirectoryImpl implements
RaftStorageDirectory {
}
// check enough space
- if (!hasEnoughSpace()) {
- LOG.warn("There are not enough space left for directory {}"
- + " free space min required: {} free space actual: {}",
- rootPath, freeSpaceMin, root.getFreeSpace());
+ final long freeSpace = root.getFreeSpace();
+ if (freeSpace < freeSpaceMin.getSize()) {
+ LOG.warn("{} in directory {}: free space = {} < required = {}",
+ StorageState.NO_SPACE, rootPath, freeSpace, freeSpaceMin);
return StorageState.NO_SPACE;
}
@@ -177,10 +177,6 @@ class RaftStorageDirectoryImpl implements
RaftStorageDirectory {
return getMetaFile().exists();
}
- private boolean hasEnoughSpace() {
- return root.getFreeSpace() >= freeSpaceMin.getSize();
- }
-
/**
* Lock storage to provide exclusive access.
*
diff --git
a/ratis-server/src/test/java/org/apache/ratis/server/simulation/SimulatedServerRpc.java
b/ratis-server/src/test/java/org/apache/ratis/server/simulation/SimulatedServerRpc.java
index 863432d9c..85e2620a5 100644
---
a/ratis-server/src/test/java/org/apache/ratis/server/simulation/SimulatedServerRpc.java
+++
b/ratis-server/src/test/java/org/apache/ratis/server/simulation/SimulatedServerRpc.java
@@ -211,8 +211,7 @@ class SimulatedServerRpc implements RaftServerRpc {
:
IOUtils.asIOException(JavaUtils.unwrapCompletionException(exception));
clientHandler.getRpc().sendReply(request, reply, e);
} catch (IOException e) {
- LOG.warn("Failed to send reply {} for request {} due to exception
{}",
- reply, request, e);
+ LOG.warn("Failed to send reply {} for request {}", reply, request,
e);
}
}, executor);
return null;
diff --git
a/ratis-test/src/test/java/org/apache/ratis/util/TestPeerProxyMap.java
b/ratis-test/src/test/java/org/apache/ratis/util/TestPeerProxyMap.java
index 62ec35400..549fbc53f 100644
--- a/ratis-test/src/test/java/org/apache/ratis/util/TestPeerProxyMap.java
+++ b/ratis-test/src/test/java/org/apache/ratis/util/TestPeerProxyMap.java
@@ -24,6 +24,9 @@ import org.junit.Assert;
import org.junit.Test;
import java.io.Closeable;
+import java.io.IOException;
+import java.net.ConnectException;
+import java.net.SocketAddress;
/** Tests for {@link PeerProxyMap}. */
public class TestPeerProxyMap extends BaseTest {
@@ -77,4 +80,50 @@ public class TestPeerProxyMap extends BaseTest {
map.resetProxy(id); // hold resetLock and then call close() with requires
proxy lock
t.join();
}
+
+ /** Copied from {@link
org.apache.ratis.thirdparty.io.netty.channel.AbstractChannel}. */
+ private static final class AnnotatedConnectException extends
ConnectException {
+ private static final long serialVersionUID = 3901958112696433556L;
+
+ AnnotatedConnectException(ConnectException exception, SocketAddress
remoteAddress) {
+ super(exception.getMessage() + ": " + remoteAddress);
+ initCause(exception);
+ }
+
+ // Suppress a warning since this method doesn't need synchronization
+ @Override
+ public Throwable fillInStackTrace() {
+ return this;
+ }
+ }
+
+ private static class ExceptionProxy implements Closeable {
+ private final RaftPeer peer;
+
+ ExceptionProxy(RaftPeer peer) {
+ this.peer = peer;
+ }
+
+ @Override
+ public void close() throws IOException {
+ throw new AnnotatedConnectException(new ConnectException("Failed to
connect to " + peer.getId()), null);
+ }
+
+ @Override
+ public String toString() {
+ return peer.getId().toString();
+ }
+ }
+
+ @Test(timeout = 1000)
+ public void testStackTrace() {
+ final RaftPeerId id = RaftPeerId.valueOf("s0");
+ final RaftPeer peer = RaftPeer.newBuilder().setId(id).build();
+ try(final PeerProxyMap<ExceptionProxy> map = new PeerProxyMap<>("test",
ExceptionProxy::new);
+ final ExceptionProxy ignored = map.computeIfAbsent(peer).get()) {
+ } catch (IOException e) {
+ assertThrowable("closeProxy", e, AnnotatedConnectException.class, LOG,
ConnectException.class);
+ Assert.assertEquals(0, e.getStackTrace().length);
+ }
+ }
}
