You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/06/18 21:09:53 UTC
[GitHub] [ignite-3] sashapolo commented on a change in pull request #176: IGNITE-14853
sashapolo commented on a change in pull request #176:
URL: https://github.com/apache/ignite-3/pull/176#discussion_r654408745
##########
File path: modules/raft/src/main/java/org/apache/ignite/raft/jraft/core/NodeImpl.java
##########
@@ -1992,6 +1992,11 @@ public Message handleAppendEntriesRequest(final AppendEntriesRequest request, fi
checkAndSetConfiguration(true);
return null;
}
+ catch (Throwable t) {
+ LOG.error("Exception", t);
Review comment:
why is this line needed? Won't this exception be logged elsewhere?
##########
File path: modules/raft/src/test/java/org/apache/ignite/raft/jraft/core/TestCluster.java
##########
@@ -47,17 +47,25 @@
import org.apache.ignite.raft.jraft.util.Endpoint;
import org.apache.ignite.raft.jraft.util.Utils;
import org.jetbrains.annotations.Nullable;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;
/**
* Test cluster for NodeTest
*/
public class TestCluster {
- /** Default election timeout. */
- private static final int ELECTION_TIMEOUT = 300;
+ /**
+ * Default election timeout.
+ * Important: due to sync disk ops (writing raft meta) during probe request processing this timeout should be high
+ * enough to avoid test flakiness.
+ */
+ private static final int ELECTION_TIMEOUT = 600;
Review comment:
I think it is always nice to use the time units as part of the variable name, for example: `ELECTION_TIMEOUT_SECONDS`
##########
File path: modules/raft/src/main/java/org/apache/ignite/raft/jraft/util/internal/ThrowUtil.java
##########
@@ -46,17 +38,50 @@ public static void throwException(final Throwable t) {
throw (E) t;
}
- public static <T extends Throwable> T cutCause(final T cause) {
- Throwable rootCause = cause;
- while (rootCause.getCause() != null) {
- rootCause = rootCause.getCause();
- }
+ /**
+ * Checks if passed in {@code 'Throwable'} has given class in {@code 'cause'} hierarchy
+ * <b>including</b> that throwable itself.
+ * <p>
+ * Note that this method follows includes {@link Throwable#getSuppressed()}
+ * into check.
+ *
+ * @param t Throwable to check (if {@code null}, {@code false} is returned).
+ * @param msg Message text that should be in cause.
+ * @param cls Cause classes to check (if {@code null} or empty, {@code false} is returned).
+ * @return {@code True} if one of the causing exception is an instance of passed in classes,
+ * {@code false} otherwise.
+ */
+ @SafeVarargs
+ public static boolean hasCause(@Nullable Throwable t, @Nullable String msg, @Nullable Class<?>... cls) {
Review comment:
`@Nullable Class<?>... cls` - this notation actually means "an array of Class instances that can *contain* nulls", which doesn't make a lot of sense in this method
##########
File path: modules/raft/src/test/java/org/apache/ignite/raft/jraft/test/TestUtils.java
##########
@@ -90,29 +88,11 @@ public static LogEntry mockEntry(final int index, final int term, final int data
}
public static String getMyIp() {
- String ip = null;
try {
- Enumeration<NetworkInterface> interfaces = NetworkInterface.getNetworkInterfaces();
- while (interfaces.hasMoreElements()) {
- NetworkInterface iface = interfaces.nextElement();
- // filters out 127.0.0.1 and inactive interfaces
- if (iface.isLoopback() || !iface.isUp()) {
- continue;
- }
-
- Enumeration<InetAddress> addresses = iface.getInetAddresses();
- while (addresses.hasMoreElements()) {
- InetAddress addr = addresses.nextElement();
- if (addr instanceof Inet4Address) {
- ip = addr.getHostAddress();
- break;
- }
- }
- }
- return ip;
+ return Inet4Address.getLocalHost().getHostAddress();
Review comment:
This diff looks weird, shoudn't this code already be in `main`?
##########
File path: modules/raft/src/test/java/org/apache/ignite/raft/jraft/core/TestCluster.java
##########
@@ -47,17 +47,25 @@
import org.apache.ignite.raft.jraft.util.Endpoint;
import org.apache.ignite.raft.jraft.util.Utils;
import org.jetbrains.annotations.Nullable;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;
/**
* Test cluster for NodeTest
*/
public class TestCluster {
- /** Default election timeout. */
- private static final int ELECTION_TIMEOUT = 300;
+ /**
+ * Default election timeout.
+ * Important: due to sync disk ops (writing raft meta) during probe request processing this timeout should be high
+ * enough to avoid test flakiness.
+ */
+ private static final int ELECTION_TIMEOUT = 600;
- private static final IgniteLogger LOG = IgniteLogger.forClass(TestCluster.class);
+ private static final Logger LOG = LoggerFactory.getLogger(TestCluster.class);
Review comment:
why did you remove the `IgniteLogger`?
##########
File path: modules/raft/src/main/java/org/apache/ignite/raft/jraft/util/internal/ThrowUtil.java
##########
@@ -46,17 +38,50 @@ public static void throwException(final Throwable t) {
throw (E) t;
}
- public static <T extends Throwable> T cutCause(final T cause) {
- Throwable rootCause = cause;
- while (rootCause.getCause() != null) {
- rootCause = rootCause.getCause();
- }
+ /**
+ * Checks if passed in {@code 'Throwable'} has given class in {@code 'cause'} hierarchy
+ * <b>including</b> that throwable itself.
+ * <p>
+ * Note that this method follows includes {@link Throwable#getSuppressed()}
+ * into check.
+ *
+ * @param t Throwable to check (if {@code null}, {@code false} is returned).
+ * @param msg Message text that should be in cause.
+ * @param cls Cause classes to check (if {@code null} or empty, {@code false} is returned).
+ * @return {@code True} if one of the causing exception is an instance of passed in classes,
+ * {@code false} otherwise.
+ */
+ @SafeVarargs
Review comment:
Looks like `@SafeVarargs` annotation is not needed here
##########
File path: modules/raft/src/main/java/org/apache/ignite/raft/jraft/util/internal/ThrowUtil.java
##########
@@ -46,17 +38,50 @@ public static void throwException(final Throwable t) {
throw (E) t;
}
- public static <T extends Throwable> T cutCause(final T cause) {
- Throwable rootCause = cause;
- while (rootCause.getCause() != null) {
- rootCause = rootCause.getCause();
- }
+ /**
+ * Checks if passed in {@code 'Throwable'} has given class in {@code 'cause'} hierarchy
+ * <b>including</b> that throwable itself.
+ * <p>
+ * Note that this method follows includes {@link Throwable#getSuppressed()}
+ * into check.
+ *
+ * @param t Throwable to check (if {@code null}, {@code false} is returned).
+ * @param msg Message text that should be in cause.
+ * @param cls Cause classes to check (if {@code null} or empty, {@code false} is returned).
+ * @return {@code True} if one of the causing exception is an instance of passed in classes,
+ * {@code false} otherwise.
+ */
+ @SafeVarargs
+ public static boolean hasCause(@Nullable Throwable t, @Nullable String msg, @Nullable Class<?>... cls) {
+ if (t == null || cls == null || cls.length == 0)
+ return false;
+
+ assert cls != null;
Review comment:
this assert is redundant
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org