You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "sk0x50 (via GitHub)" <gi...@apache.org> on 2024/12/18 05:15:27 UTC
[PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
sk0x50 opened a new pull request, #4923:
URL: https://github.com/apache/ignite-3/pull/4923
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- There is a single JIRA ticket related to the pull request.
\- The web-link to the pull request is attached to the JIRA ticket.
\- The JIRA ticket has the Patch Available state.
\- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
\- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893857962
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/impl/AbstractClientService.java:
##########
@@ -233,7 +233,7 @@ public void complete(final Object result, final Throwable err) {
}
}
else {
- if (ThrowUtil.hasCause(err, null, PeerUnavailableException.class, ConnectException.class))
Review Comment:
This method was introduced into `ThrowUtil` by us (if I am not mistaken). So, I decided to remove duplicates. If you think that it is a bad idea and we should not modify this code, then I will revert this change. No problems.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893847206
##########
modules/failure-handler/src/main/java/org/apache/ignite/internal/failure/FailureManager.java:
##########
@@ -225,4 +271,36 @@ public synchronized void setInterceptor(@Nullable FailureHandler interceptor) {
FailureHandler handler() {
return handler;
}
+
+ /**
+ * Defines whether thread dump should be throttled for given failure type or not.
+ *
+ * @param type Failure type.
+ * @return {@code true} if thread dump generation should be throttled for given failure type.
+ */
+ private boolean throttleThreadDump(FailureType type) {
+ Map<FailureType, Long> dumpPerFailureTypeTs = threadDumpPerFailureTypeTs;
+ long dumpThrottlingTimeout = dumpThreadsThrottlingTimeout;
+
+ if (dumpThrottlingTimeout == 0 || dumpPerFailureTypeTs == null) {
+ return false;
+ }
+
+ long curr = System.currentTimeMillis();
+
+ Long last = dumpPerFailureTypeTs.get(type);
+
+ assert last != null : "Unknown failure type " + type;
+
+ boolean throttle = curr - last < dumpThrottlingTimeout;
+
+ if (!throttle) {
+ dumpPerFailureTypeTs.put(type, curr);
Review Comment:
The `throttleThreadDump` is called by `process` method which is `synchronized`. So, it looks OK.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893925354
##########
modules/failure-handler/src/main/java/org/apache/ignite/internal/failure/FailureManager.java:
##########
@@ -225,4 +271,36 @@ public synchronized void setInterceptor(@Nullable FailureHandler interceptor) {
FailureHandler handler() {
return handler;
}
+
+ /**
+ * Defines whether thread dump should be throttled for given failure type or not.
+ *
+ * @param type Failure type.
+ * @return {@code true} if thread dump generation should be throttled for given failure type.
+ */
+ private boolean throttleThreadDump(FailureType type) {
+ Map<FailureType, Long> dumpPerFailureTypeTs = threadDumpPerFailureTypeTs;
+ long dumpThrottlingTimeout = dumpThreadsThrottlingTimeout;
+
+ if (dumpThrottlingTimeout == 0 || dumpPerFailureTypeTs == null) {
+ return false;
+ }
+
+ long curr = System.currentTimeMillis();
+
+ Long last = dumpPerFailureTypeTs.get(type);
+
+ assert last != null : "Unknown failure type " + type;
+
+ boolean throttle = curr - last < dumpThrottlingTimeout;
+
+ if (!throttle) {
+ dumpPerFailureTypeTs.put(type, curr);
Review Comment:
Does it make sense to say in its javadoc that it should be called under synchronization?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893721055
##########
modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java:
##########
@@ -127,6 +136,16 @@ public static long monotonicMs() {
*/
private static final String JMX_MBEAN_PACKAGE = "org.apache";
+ /** Thread dump message. */
+ public static final String THREAD_DUMP_MSG = "Thread dump at ";
+
+ /** Date format for thread dumps. */
+ private static final DateTimeFormatter THREAD_DUMP_FMT =
+ DateTimeFormatter.ofPattern("yyyy/MM/dd HH:mm:ss z").withZone(ZoneId.systemDefault());
Review Comment:
I don't have any strong arguments for or against it, except that Apache Ignite 2 uses this format. Ok will change.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893857962
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/impl/AbstractClientService.java:
##########
@@ -233,7 +233,7 @@ public void complete(final Object result, final Throwable err) {
}
}
else {
- if (ThrowUtil.hasCause(err, null, PeerUnavailableException.class, ConnectException.class))
Review Comment:
This method was introduced into `ThrowUtil` from JRaft. So, I decided to remove duplicates. If you think that it is a bad idea and we should not modify this code, then I will revert this change. No problems.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893911882
##########
modules/core/src/main/java/org/apache/ignite/internal/util/ExceptionUtils.java:
##########
@@ -332,6 +332,52 @@ public static String getFullStackTrace(Throwable throwable) {
return sw.getBuffer().toString();
}
+ /**
+ * Checks if passed in {@code 'Throwable'} has given class in {@code 'cause'} hierarchy
+ * <b>including</b> that throwable itself.
+ * 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.
+ */
+ public static boolean hasCause(@Nullable Throwable t, @Nullable String msg, Class<?> @Nullable... cls) {
Review Comment:
>It seems to me, that this particular test method cannot be applied in our case.
Certainly, I did not suggest to use it directly :) I just wanted to say that this terminology (causeOrSuppressed) is already in the code
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893701006
##########
modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java:
##########
@@ -1297,4 +1316,165 @@ public static boolean shouldSwitchToRequestsExecutor(ThreadOperation... required
return true;
}
}
+
+ /**
+ * Performs thread dump and prints all available info to the given log with WARN logging level.
+ *
+ * @param log Logger.
+ */
+ public static void dumpThreads(IgniteLogger log) {
+ dumpThreads(log, false);
+ }
+
+ /**
+ * Performs thread dump and prints all available info to the given log
+ * with WARN or ERROR logging level depending on {@code isErrorLevel} parameter.
+ *
+ * @param log Logger.
+ * @param isErrorLevel {@code true} if thread dump must be printed with ERROR logging level,
+ * {@code false} if thread dump must be printed with WARN logging level.
+ */
+ public static void dumpThreads(IgniteLogger log, boolean isErrorLevel) {
+ ThreadMXBean mxBean = ManagementFactory.getThreadMXBean();
+
+ Set<Long> deadlockedThreadsIds = getDeadlockedThreadIds(mxBean);
+
+ if (deadlockedThreadsIds.isEmpty()) {
+ logMessage(log, "No deadlocked threads detected.", isErrorLevel);
+ } else {
+ logMessage(log, "Deadlocked threads detected (see thread dump below) "
+ + "[deadlockedThreadsCnt=" + deadlockedThreadsIds.size() + ']', isErrorLevel);
Review Comment:
I always forget that the Ignite 3 code style does not require using abbreviations. :)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893820469
##########
modules/core/src/main/java/org/apache/ignite/internal/util/ExceptionUtils.java:
##########
@@ -332,6 +332,52 @@ public static String getFullStackTrace(Throwable throwable) {
return sw.getBuffer().toString();
}
+ /**
+ * Checks if passed in {@code 'Throwable'} has given class in {@code 'cause'} hierarchy
+ * <b>including</b> that throwable itself.
+ * 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.
+ */
+ public static boolean hasCause(@Nullable Throwable t, @Nullable String msg, Class<?> @Nullable... cls) {
Review Comment:
Well, it is just a copy of the Apache Ignite 2 code. Of course, it does not mean that we cannot change it! I like the proposed `hasCauseOrSuppressed`.
> Just a note: we have willThrowWithCauseOrSuppressed() in the codebase.
It seems to me, that this particular test method cannot be applied in our case. So, I would stay with a new one (introduced in the patch).
> Also, is it possible to rename parameters and variables to make them more human-readable?
I will do it. Thanks for pointing this out.
> And: this doesn't seem to tolerate cycles in the exception graph (except for a special case when cause equals the exception itself). I think we sometimes add suppressions in a way that creates such cycles. Should they be detected? I think I saw code for their detection somewhere (maybe it was in some test-only utility class).
Do you mean something like as follows https://github.com/apache/ignite-3/blob/main/modules/api/src/main/java/org/apache/ignite/lang/util/TraceIdUtils.java#L42 ?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893530328
##########
modules/catalog-compaction/src/integrationTest/java/org/apache/ignite/internal/catalog/compaction/ItCatalogCompactionTest.java:
##########
@@ -98,7 +98,8 @@ protected String getNodeBootstrapConfigTemplate() {
+ " },\n"
+ " clientConnector.port: {},\n"
+ " rest.port: {},\n"
- + " compute.threadPoolSize: 1\n"
+ + " compute.threadPoolSize: 1,\n"
+ + " failureHandler.dumpThreadsOnFailure: false\n"
Review Comment:
Why is this disabled everywhere in tests?
##########
modules/core/src/main/java/org/apache/ignite/internal/util/ExceptionUtils.java:
##########
@@ -332,6 +332,52 @@ public static String getFullStackTrace(Throwable throwable) {
return sw.getBuffer().toString();
}
+ /**
+ * Checks if passed in {@code 'Throwable'} has given class in {@code 'cause'} hierarchy
+ * <b>including</b> that throwable itself.
+ * 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.
+ */
+ public static boolean hasCause(@Nullable Throwable t, @Nullable String msg, Class<?> @Nullable... cls) {
Review Comment:
The name seems to be misleading: it only says about cause, but the method actually takes suppressed into consideration. The javadoc explains this, but it seems more important to have the name that tells truth :) How about `hasCauseOrSuppressed()` or `hasRelated()`?
Just a note: we have `willThrowWithCauseOrSuppressed()` in the codebase.
Also, is it possible to rename parameters and variables to make them more human-readable?
And: this doesn't seem to tolerate cycles in the exception graph (except for a special case when cause equals the exception itself). I think we sometimes add suppressions in a way that creates such cycles. Should they be detected? I think I saw code for their detection somewhere (maybe it was in some test-only utility class).
##########
modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java:
##########
@@ -1297,4 +1316,165 @@ public static boolean shouldSwitchToRequestsExecutor(ThreadOperation... required
return true;
}
}
+
+ /**
+ * Performs thread dump and prints all available info to the given log with WARN logging level.
+ *
+ * @param log Logger.
+ */
+ public static void dumpThreads(IgniteLogger log) {
+ dumpThreads(log, false);
+ }
+
+ /**
+ * Performs thread dump and prints all available info to the given log
+ * with WARN or ERROR logging level depending on {@code isErrorLevel} parameter.
+ *
+ * @param log Logger.
+ * @param isErrorLevel {@code true} if thread dump must be printed with ERROR logging level,
+ * {@code false} if thread dump must be printed with WARN logging level.
+ */
+ public static void dumpThreads(IgniteLogger log, boolean isErrorLevel) {
+ ThreadMXBean mxBean = ManagementFactory.getThreadMXBean();
+
+ Set<Long> deadlockedThreadsIds = getDeadlockedThreadIds(mxBean);
+
+ if (deadlockedThreadsIds.isEmpty()) {
+ logMessage(log, "No deadlocked threads detected.", isErrorLevel);
+ } else {
+ logMessage(log, "Deadlocked threads detected (see thread dump below) "
+ + "[deadlockedThreadsCnt=" + deadlockedThreadsIds.size() + ']', isErrorLevel);
Review Comment:
```suggestion
+ "[deadlockedThreadsCount=" + deadlockedThreadsIds.size() + ']', isErrorLevel);
```
##########
modules/failure-handler/src/main/java/org/apache/ignite/internal/failure/FailureManager.java:
##########
@@ -168,6 +196,24 @@ private void initFailureHandler() {
return;
}
+ dumpThreadsOnFailure = configuration.dumpThreadsOnFailure().value();
Review Comment:
Would it make sense to make these properties dynamic (that is, listen to their changes)?
##########
modules/failure-handler/src/main/java/org/apache/ignite/internal/failure/FailureManager.java:
##########
@@ -119,7 +137,7 @@ public FailureContext failureContext() {
* Processes failure accordingly to configured {@link FailureHandler}.
*
* @param failureCtx Failure context.
- * @return {@code True} If this very call led to Ignite node invalidation.
+ * @return {@code true} If this call leads to Ignite node invalidation and {@code false} otherwise.
Review Comment:
Do we need this javadoc at all? It seems to be no different from the javadoc on the interface method
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/impl/AbstractClientService.java:
##########
@@ -233,7 +233,7 @@ public void complete(final Object result, final Throwable err) {
}
}
else {
- if (ThrowUtil.hasCause(err, null, PeerUnavailableException.class, ConnectException.class))
Review Comment:
We are trying to minimize changes to JRaft code. Do we really need to move the method?
##########
modules/failure-handler/src/test/java/org/apache/ignite/internal/failure/FailureProcessorThreadDumpThrottlingTest.java:
##########
@@ -0,0 +1,232 @@
+/*
+ * 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.ignite.internal.failure;
+
+import static org.apache.ignite.internal.failure.FailureType.SYSTEM_CRITICAL_OPERATION_TIMEOUT;
+import static org.apache.ignite.internal.failure.FailureType.SYSTEM_WORKER_BLOCKED;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willSucceedFast;
+import static org.apache.ignite.internal.util.IgniteUtils.THREAD_DUMP_MSG;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.Consumer;
+import org.apache.ignite.internal.configuration.testframework.ConfigurationExtension;
+import org.apache.ignite.internal.configuration.testframework.InjectConfiguration;
+import org.apache.ignite.internal.failure.configuration.FailureProcessorConfiguration;
+import org.apache.ignite.internal.manager.ComponentContext;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.apache.ignite.internal.testframework.log4j2.LogInspector;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/**
+ * Tests for {@link FailureProcessor} thread dump throttling.
+ */
+@ExtendWith(ConfigurationExtension.class)
+public class FailureProcessorThreadDumpThrottlingTest extends BaseIgniteAbstractTest {
+ @InjectConfiguration("mock: { "
+ + "oomBufferSizeBites=0, "
+ + "dumpThreadsOnFailure=true,"
+ + "dumpThreadsThrottlingTimeoutMillis=0,"
+ + "handler {type=noop} }")
+ private static FailureProcessorConfiguration enabledThreadDumpNoThrottlingConfiguration;
+
+ @InjectConfiguration("mock: { "
+ + "oomBufferSizeBites=0, "
+ + "dumpThreadsOnFailure=true,"
+ + "dumpThreadsThrottlingTimeoutMillis=3000,"
+ + "handler {type=noop} }")
+ private static FailureProcessorConfiguration enabledThreadDumpWithThrottlingConfiguration;
+
+ @InjectConfiguration("mock: { "
+ + "oomBufferSizeBites=0, "
+ + "dumpThreadsOnFailure=false,"
+ + "dumpThreadsThrottlingTimeoutMillis=0,"
+ + "handler {type=noop} }")
+ private static FailureProcessorConfiguration disabledThreadDumpConfiguration;
+
+ /**
+ * Tests that thread dumps will not get if 'dumpThreadsOnFailure' equals to `false`.
+ */
+ @Test
+ public void testNoThreadDumps() {
+ LogInspector logInspector = new LogInspector(
+ FailureManager.class.getName(),
+ evt -> evt.getMessage().getFormattedMessage().startsWith(THREAD_DUMP_MSG));
+
+ logInspector.start();
+ try {
+ FailureManager failureManager = new FailureManager(() -> {}, disabledThreadDumpConfiguration);
+
+ try {
+ assertThat(failureManager.startAsync(new ComponentContext()), willSucceedFast());
+
+ FailureContext failureCtx = new FailureContext(SYSTEM_WORKER_BLOCKED, new Throwable("Failure context error"));
+
+ assertThat(failureManager.process(failureCtx), is(false));
+ } finally {
+ assertThat(failureManager.stopAsync(new ComponentContext()), willSucceedFast());
+ }
+ } finally {
+ logInspector.stop();
+ }
+
+ assertThat(logInspector.isMatched(), is(false));
+ }
+
+ /**
+ * Tests that thread dumps will get for every failure for disabled throttling.
Review Comment:
```suggestion
* Tests that thread dumps will get for every failure when throttling is disabled.
```
##########
modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java:
##########
@@ -1297,4 +1316,165 @@ public static boolean shouldSwitchToRequestsExecutor(ThreadOperation... required
return true;
}
}
+
+ /**
+ * Performs thread dump and prints all available info to the given log with WARN logging level.
+ *
+ * @param log Logger.
+ */
+ public static void dumpThreads(IgniteLogger log) {
Review Comment:
Do we want to have this method in the overly generic `IgniteUtils`? How about creating something like `ThreadUtils` for it? In AI2 `IgniteUtils` is so huge that it makes IDEs work really slow, navigation is pretty difficult. I think we should modularize this.
##########
modules/failure-handler/src/main/java/org/apache/ignite/internal/failure/FailureManager.java:
##########
@@ -225,4 +271,36 @@ public synchronized void setInterceptor(@Nullable FailureHandler interceptor) {
FailureHandler handler() {
return handler;
}
+
+ /**
+ * Defines whether thread dump should be throttled for given failure type or not.
+ *
+ * @param type Failure type.
+ * @return {@code true} if thread dump generation should be throttled for given failure type.
+ */
+ private boolean throttleThreadDump(FailureType type) {
+ Map<FailureType, Long> dumpPerFailureTypeTs = threadDumpPerFailureTypeTs;
+ long dumpThrottlingTimeout = dumpThreadsThrottlingTimeout;
+
+ if (dumpThrottlingTimeout == 0 || dumpPerFailureTypeTs == null) {
+ return false;
+ }
+
+ long curr = System.currentTimeMillis();
+
+ Long last = dumpPerFailureTypeTs.get(type);
+
+ assert last != null : "Unknown failure type " + type;
+
+ boolean throttle = curr - last < dumpThrottlingTimeout;
+
+ if (!throttle) {
+ dumpPerFailureTypeTs.put(type, curr);
Review Comment:
`EnumMap` is not thread safe. How is this going to work?
##########
modules/failure-handler/src/main/java/org/apache/ignite/internal/failure/configuration/FailureProcessorConfigurationSchema.java:
##########
@@ -19,13 +19,34 @@
import org.apache.ignite.configuration.annotation.Config;
import org.apache.ignite.configuration.annotation.ConfigValue;
+import org.apache.ignite.configuration.annotation.Value;
+import org.apache.ignite.configuration.validation.Range;
import org.apache.ignite.internal.failure.handlers.configuration.FailureHandlerConfigurationSchema;
/**
* Failure processor configuration schema.
*/
@Config
public class FailureProcessorConfigurationSchema {
+ /**
+ * Amount of memory reserved in the heap at node start in kilobytes, which can be dropped
+ * to increase the chances of success when handling OutOfMemoryError.
+ */
+ @Value(hasDefault = true)
+ @Range(min = 0)
+ public int oomBufferSizeBites = 16 * 1024;
+
+ /** Enables threads dumping on critical node failure. */
+ @Value(hasDefault = true)
+ public boolean dumpThreadsOnFailure = true;
+
+ /**
+ * Throttling time out for thread dump generation during failure handling in milliseconds.
+ * The default is 10 seconds. The {@code 0} value mens that throttling is disabled.
Review Comment:
```suggestion
* The default is 10 seconds. The {@code 0} value means that throttling is disabled.
```
##########
modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java:
##########
@@ -127,6 +136,16 @@ public static long monotonicMs() {
*/
private static final String JMX_MBEAN_PACKAGE = "org.apache";
+ /** Thread dump message. */
+ public static final String THREAD_DUMP_MSG = "Thread dump at ";
+
+ /** Date format for thread dumps. */
+ private static final DateTimeFormatter THREAD_DUMP_FMT =
+ DateTimeFormatter.ofPattern("yyyy/MM/dd HH:mm:ss z").withZone(ZoneId.systemDefault());
Review Comment:
```suggestion
DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss z").withZone(ZoneId.systemDefault());
```
1. It's closer to ISO-8601
2. Slashes might cause confusion with the US style of dates where day-in-month precedes month-in-year, but dashes don't seem to cause this assiciation
##########
modules/failure-handler/src/test/java/org/apache/ignite/internal/failure/FailureProcessorThreadDumpThrottlingTest.java:
##########
@@ -0,0 +1,232 @@
+/*
+ * 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.ignite.internal.failure;
+
+import static org.apache.ignite.internal.failure.FailureType.SYSTEM_CRITICAL_OPERATION_TIMEOUT;
+import static org.apache.ignite.internal.failure.FailureType.SYSTEM_WORKER_BLOCKED;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willSucceedFast;
+import static org.apache.ignite.internal.util.IgniteUtils.THREAD_DUMP_MSG;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.Consumer;
+import org.apache.ignite.internal.configuration.testframework.ConfigurationExtension;
+import org.apache.ignite.internal.configuration.testframework.InjectConfiguration;
+import org.apache.ignite.internal.failure.configuration.FailureProcessorConfiguration;
+import org.apache.ignite.internal.manager.ComponentContext;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.apache.ignite.internal.testframework.log4j2.LogInspector;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/**
+ * Tests for {@link FailureProcessor} thread dump throttling.
+ */
+@ExtendWith(ConfigurationExtension.class)
+public class FailureProcessorThreadDumpThrottlingTest extends BaseIgniteAbstractTest {
+ @InjectConfiguration("mock: { "
+ + "oomBufferSizeBites=0, "
+ + "dumpThreadsOnFailure=true,"
+ + "dumpThreadsThrottlingTimeoutMillis=0,"
+ + "handler {type=noop} }")
+ private static FailureProcessorConfiguration enabledThreadDumpNoThrottlingConfiguration;
+
+ @InjectConfiguration("mock: { "
+ + "oomBufferSizeBites=0, "
+ + "dumpThreadsOnFailure=true,"
+ + "dumpThreadsThrottlingTimeoutMillis=3000,"
+ + "handler {type=noop} }")
+ private static FailureProcessorConfiguration enabledThreadDumpWithThrottlingConfiguration;
+
+ @InjectConfiguration("mock: { "
+ + "oomBufferSizeBites=0, "
+ + "dumpThreadsOnFailure=false,"
+ + "dumpThreadsThrottlingTimeoutMillis=0,"
+ + "handler {type=noop} }")
+ private static FailureProcessorConfiguration disabledThreadDumpConfiguration;
+
+ /**
+ * Tests that thread dumps will not get if 'dumpThreadsOnFailure' equals to `false`.
+ */
+ @Test
+ public void testNoThreadDumps() {
+ LogInspector logInspector = new LogInspector(
+ FailureManager.class.getName(),
+ evt -> evt.getMessage().getFormattedMessage().startsWith(THREAD_DUMP_MSG));
+
+ logInspector.start();
+ try {
+ FailureManager failureManager = new FailureManager(() -> {}, disabledThreadDumpConfiguration);
+
+ try {
+ assertThat(failureManager.startAsync(new ComponentContext()), willSucceedFast());
+
+ FailureContext failureCtx = new FailureContext(SYSTEM_WORKER_BLOCKED, new Throwable("Failure context error"));
+
+ assertThat(failureManager.process(failureCtx), is(false));
+ } finally {
+ assertThat(failureManager.stopAsync(new ComponentContext()), willSucceedFast());
+ }
+ } finally {
+ logInspector.stop();
+ }
+
+ assertThat(logInspector.isMatched(), is(false));
+ }
+
+ /**
+ * Tests that thread dumps will get for every failure for disabled throttling.
Review Comment:
```suggestion
* Tests that thread dumps will be logged for every failure for disabled throttling.
```
##########
modules/failure-handler/src/test/java/org/apache/ignite/internal/failure/FailureProcessorThreadDumpThrottlingTest.java:
##########
@@ -0,0 +1,232 @@
+/*
+ * 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.ignite.internal.failure;
+
+import static org.apache.ignite.internal.failure.FailureType.SYSTEM_CRITICAL_OPERATION_TIMEOUT;
+import static org.apache.ignite.internal.failure.FailureType.SYSTEM_WORKER_BLOCKED;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willSucceedFast;
+import static org.apache.ignite.internal.util.IgniteUtils.THREAD_DUMP_MSG;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.Consumer;
+import org.apache.ignite.internal.configuration.testframework.ConfigurationExtension;
+import org.apache.ignite.internal.configuration.testframework.InjectConfiguration;
+import org.apache.ignite.internal.failure.configuration.FailureProcessorConfiguration;
+import org.apache.ignite.internal.manager.ComponentContext;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.apache.ignite.internal.testframework.log4j2.LogInspector;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/**
+ * Tests for {@link FailureProcessor} thread dump throttling.
+ */
+@ExtendWith(ConfigurationExtension.class)
+public class FailureProcessorThreadDumpThrottlingTest extends BaseIgniteAbstractTest {
+ @InjectConfiguration("mock: { "
+ + "oomBufferSizeBites=0, "
+ + "dumpThreadsOnFailure=true,"
+ + "dumpThreadsThrottlingTimeoutMillis=0,"
+ + "handler {type=noop} }")
+ private static FailureProcessorConfiguration enabledThreadDumpNoThrottlingConfiguration;
+
+ @InjectConfiguration("mock: { "
+ + "oomBufferSizeBites=0, "
+ + "dumpThreadsOnFailure=true,"
+ + "dumpThreadsThrottlingTimeoutMillis=3000,"
+ + "handler {type=noop} }")
+ private static FailureProcessorConfiguration enabledThreadDumpWithThrottlingConfiguration;
+
+ @InjectConfiguration("mock: { "
+ + "oomBufferSizeBites=0, "
+ + "dumpThreadsOnFailure=false,"
+ + "dumpThreadsThrottlingTimeoutMillis=0,"
+ + "handler {type=noop} }")
+ private static FailureProcessorConfiguration disabledThreadDumpConfiguration;
+
+ /**
+ * Tests that thread dumps will not get if 'dumpThreadsOnFailure' equals to `false`.
Review Comment:
```suggestion
* Tests that thread dumps will not be logged if 'dumpThreadsOnFailure' equals to `false`.
```
##########
modules/failure-handler/src/test/java/org/apache/ignite/internal/failure/FailureProcessorThreadDumpThrottlingTest.java:
##########
@@ -0,0 +1,232 @@
+/*
+ * 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.ignite.internal.failure;
+
+import static org.apache.ignite.internal.failure.FailureType.SYSTEM_CRITICAL_OPERATION_TIMEOUT;
+import static org.apache.ignite.internal.failure.FailureType.SYSTEM_WORKER_BLOCKED;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willSucceedFast;
+import static org.apache.ignite.internal.util.IgniteUtils.THREAD_DUMP_MSG;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.Consumer;
+import org.apache.ignite.internal.configuration.testframework.ConfigurationExtension;
+import org.apache.ignite.internal.configuration.testframework.InjectConfiguration;
+import org.apache.ignite.internal.failure.configuration.FailureProcessorConfiguration;
+import org.apache.ignite.internal.manager.ComponentContext;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.apache.ignite.internal.testframework.log4j2.LogInspector;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/**
+ * Tests for {@link FailureProcessor} thread dump throttling.
+ */
+@ExtendWith(ConfigurationExtension.class)
+public class FailureProcessorThreadDumpThrottlingTest extends BaseIgniteAbstractTest {
+ @InjectConfiguration("mock: { "
+ + "oomBufferSizeBites=0, "
+ + "dumpThreadsOnFailure=true,"
+ + "dumpThreadsThrottlingTimeoutMillis=0,"
+ + "handler {type=noop} }")
+ private static FailureProcessorConfiguration enabledThreadDumpNoThrottlingConfiguration;
+
+ @InjectConfiguration("mock: { "
+ + "oomBufferSizeBites=0, "
+ + "dumpThreadsOnFailure=true,"
+ + "dumpThreadsThrottlingTimeoutMillis=3000,"
+ + "handler {type=noop} }")
+ private static FailureProcessorConfiguration enabledThreadDumpWithThrottlingConfiguration;
+
+ @InjectConfiguration("mock: { "
+ + "oomBufferSizeBites=0, "
+ + "dumpThreadsOnFailure=false,"
+ + "dumpThreadsThrottlingTimeoutMillis=0,"
+ + "handler {type=noop} }")
+ private static FailureProcessorConfiguration disabledThreadDumpConfiguration;
+
+ /**
+ * Tests that thread dumps will not get if 'dumpThreadsOnFailure' equals to `false`.
+ */
+ @Test
+ public void testNoThreadDumps() {
+ LogInspector logInspector = new LogInspector(
+ FailureManager.class.getName(),
+ evt -> evt.getMessage().getFormattedMessage().startsWith(THREAD_DUMP_MSG));
+
+ logInspector.start();
+ try {
+ FailureManager failureManager = new FailureManager(() -> {}, disabledThreadDumpConfiguration);
+
+ try {
+ assertThat(failureManager.startAsync(new ComponentContext()), willSucceedFast());
+
+ FailureContext failureCtx = new FailureContext(SYSTEM_WORKER_BLOCKED, new Throwable("Failure context error"));
+
+ assertThat(failureManager.process(failureCtx), is(false));
+ } finally {
+ assertThat(failureManager.stopAsync(new ComponentContext()), willSucceedFast());
+ }
+ } finally {
+ logInspector.stop();
+ }
+
+ assertThat(logInspector.isMatched(), is(false));
+ }
+
+ /**
+ * Tests that thread dumps will get for every failure for disabled throttling.
+ */
+ @Test
+ public void testNoThrottling() {
+ AtomicInteger messageCounter = new AtomicInteger();
+
+ LogInspector logInspector = new LogInspector(
+ FailureManager.class.getName(),
+ evt -> evt.getMessage().getFormattedMessage().startsWith(THREAD_DUMP_MSG),
+ messageCounter::incrementAndGet);
+
+ logInspector.start();
+ try {
+ testFailureProcessing(enabledThreadDumpNoThrottlingConfiguration, failureProcessor -> {
+ FailureContext failureCtx = new FailureContext(SYSTEM_WORKER_BLOCKED, new Throwable("Failure context error"));
+
+ // Trigger the failure processing twice.
+ assertThat(failureProcessor.process(failureCtx), is(false));
+
+ assertThat(failureProcessor.process(failureCtx), is(false));
+ });
+ } finally {
+ logInspector.stop();
+ }
+
+ assertThat(messageCounter.get(), is(2));
+ }
+
+ /**
+ * Tests that thread dumps will be throttled and will be generated again after timeout exceeded.
+ */
+ @Test
+ public void testThrottling() {
+ AtomicInteger threadDumpMessageCounter = new AtomicInteger();
+ AtomicInteger throttlingMessageCounter = new AtomicInteger();
+
+ LogInspector logInspector = new LogInspector(FailureManager.class.getName());
+
+ logInspector.addHandler(
+ evt -> evt.getMessage().getFormattedMessage().startsWith(THREAD_DUMP_MSG),
+ threadDumpMessageCounter::incrementAndGet);
+ logInspector.addHandler(
+ evt -> evt.getMessage().getFormattedMessage().startsWith("Thread dump is hidden due to throttling settings."),
+ throttlingMessageCounter::incrementAndGet);
+
+ logInspector.start();
+ try {
+ testFailureProcessing(enabledThreadDumpWithThrottlingConfiguration, failureProcessor -> {
+ FailureContext failureCtx = new FailureContext(SYSTEM_WORKER_BLOCKED, new Throwable("Failure context error"));
+
+ // Trigger the failure processing twice.
+ for (int i = 0; i < 2; i++) {
+ assertThat(failureProcessor.process(failureCtx), is(false));
+ }
+
+ try {
+ Thread.sleep(3000);
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+
+ // Trigger the failure processing twice.
+ for (int i = 0; i < 2; i++) {
+ assertThat(failureProcessor.process(failureCtx), is(false));
+ }
+ });
+ } finally {
+ logInspector.stop();
+ }
+
+ assertThat(threadDumpMessageCounter.get(), is(2));
+ assertThat(throttlingMessageCounter.get(), is(2));
+ }
+
+ /**
+ * Tests that thread dumps will be throttled per failure type and will be generated again after timeout exceeded.
+ */
+ @Test
+ public void testThrottlingPerFailureType() {
+ AtomicInteger threadDumpMessageCounter = new AtomicInteger();
+ AtomicInteger throttlingMessageCounter = new AtomicInteger();
+
+ LogInspector logInspector = new LogInspector(FailureManager.class.getName());
+
+ logInspector.addHandler(
+ evt -> evt.getMessage().getFormattedMessage().startsWith(THREAD_DUMP_MSG),
+ threadDumpMessageCounter::incrementAndGet);
+ logInspector.addHandler(
+ evt -> evt.getMessage().getFormattedMessage().startsWith("Thread dump is hidden due to throttling settings."),
+ throttlingMessageCounter::incrementAndGet);
+
+ logInspector.start();
+ try {
+ testFailureProcessing(enabledThreadDumpWithThrottlingConfiguration, failureProcessor -> {
+ FailureContext workerBlockedFailureCtx =
+ new FailureContext(SYSTEM_WORKER_BLOCKED, new Throwable("Failure context error"));
+
+ FailureContext opTimeoutFailureCtx =
+ new FailureContext(SYSTEM_CRITICAL_OPERATION_TIMEOUT, new Throwable("Failure context error"));
+
+ for (int i = 0; i < 2; i++) {
+ failureProcessor.process(workerBlockedFailureCtx);
+ failureProcessor.process(opTimeoutFailureCtx);
+ }
+
+ try {
+ Thread.sleep(3000);
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+
+ for (int i = 0; i < 2; i++) {
+ failureProcessor.process(workerBlockedFailureCtx);
+ failureProcessor.process(opTimeoutFailureCtx);
+ }
+ });
+ } finally {
+ logInspector.stop();
+ }
+
+ assertThat(threadDumpMessageCounter.get(), is(4));
+ assertThat(throttlingMessageCounter.get(), is(4));
+ }
+
+ /**
+ * Creates a new instance of {@link FailureManager} with the given configuration and runs the test represented by {@code test} closure.
+ */
+ public static void testFailureProcessing(FailureProcessorConfiguration configuration, Consumer<FailureProcessor> test) {
Review Comment:
It is very unusual to have a public method that is not test in a test class. Does it need to be public?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893857962
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/impl/AbstractClientService.java:
##########
@@ -233,7 +233,7 @@ public void complete(final Object result, final Throwable err) {
}
}
else {
- if (ThrowUtil.hasCause(err, null, PeerUnavailableException.class, ConnectException.class))
Review Comment:
This method was introduced into `ThrowUtil` by us. So, I decided to remove duplicates. If you think that it is a bad idea and we should not modify this code, then I will revert this change. No problems.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893910645
##########
modules/catalog-compaction/src/integrationTest/java/org/apache/ignite/internal/catalog/compaction/ItCatalogCompactionTest.java:
##########
@@ -98,7 +98,8 @@ protected String getNodeBootstrapConfigTemplate() {
+ " },\n"
+ " clientConnector.port: {},\n"
+ " rest.port: {},\n"
- + " compute.threadPoolSize: 1\n"
+ + " compute.threadPoolSize: 1,\n"
+ + " failureHandler.dumpThreadsOnFailure: false\n"
Review Comment:
I see. But should we just enable log analyzer that fails a build if it sees that a failure has happened? I think Yura Gerzhedovich did such a thing; the problem was just a few tests that actually make sure that the failure happens. Maybe we should handle these tests and enable the check instead of stopping the node?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893944926
##########
modules/failure-handler/src/main/java/org/apache/ignite/internal/failure/FailureManager.java:
##########
@@ -225,4 +271,36 @@ public synchronized void setInterceptor(@Nullable FailureHandler interceptor) {
FailureHandler handler() {
return handler;
}
+
+ /**
+ * Defines whether thread dump should be throttled for given failure type or not.
+ *
+ * @param type Failure type.
+ * @return {@code true} if thread dump generation should be throttled for given failure type.
+ */
+ private boolean throttleThreadDump(FailureType type) {
+ Map<FailureType, Long> dumpPerFailureTypeTs = threadDumpPerFailureTypeTs;
+ long dumpThrottlingTimeout = dumpThreadsThrottlingTimeout;
+
+ if (dumpThrottlingTimeout == 0 || dumpPerFailureTypeTs == null) {
+ return false;
+ }
+
+ long curr = System.currentTimeMillis();
+
+ Long last = dumpPerFailureTypeTs.get(type);
+
+ assert last != null : "Unknown failure type " + type;
+
+ boolean throttle = curr - last < dumpThrottlingTimeout;
+
+ if (!throttle) {
+ dumpPerFailureTypeTs.put(type, curr);
Review Comment:
Updated.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893827884
##########
modules/core/src/main/java/org/apache/ignite/internal/util/IgniteUtils.java:
##########
@@ -1297,4 +1316,165 @@ public static boolean shouldSwitchToRequestsExecutor(ThreadOperation... required
return true;
}
}
+
+ /**
+ * Performs thread dump and prints all available info to the given log with WARN logging level.
+ *
+ * @param log Logger.
+ */
+ public static void dumpThreads(IgniteLogger log) {
Review Comment:
I thought exactly the same but did not do it... Will create `ThreadUtils`. Thank you for the suggestion/support!
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893845437
##########
modules/failure-handler/src/main/java/org/apache/ignite/internal/failure/FailureManager.java:
##########
@@ -168,6 +196,24 @@ private void initFailureHandler() {
return;
}
+ dumpThreadsOnFailure = configuration.dumpThreadsOnFailure().value();
Review Comment:
There is a separate ticket to address that https://issues.apache.org/jira/browse/IGNITE-21461
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893855193
##########
modules/catalog-compaction/src/integrationTest/java/org/apache/ignite/internal/catalog/compaction/ItCatalogCompactionTest.java:
##########
@@ -98,7 +98,8 @@ protected String getNodeBootstrapConfigTemplate() {
+ " },\n"
+ " clientConnector.port: {},\n"
+ " rest.port: {},\n"
- + " compute.threadPoolSize: 1\n"
+ + " compute.threadPoolSize: 1,\n"
+ + " failureHandler.dumpThreadsOnFailure: false\n"
Review Comment:
I did this based on Ignite 2's experience, but I think it can be changed in the future. The first step is using `stopNode` failure handler by default instead of `noop` See [IGNITE-21456](https://issues.apache.org/jira/browse/IGNITE-21456) For now, there are a lot of `green` tests on TeamCity that actually trigger failure handler, and so it will lead to huge log files that will not be analyzed (test are still `green`...)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893702707
##########
modules/failure-handler/src/main/java/org/apache/ignite/internal/failure/FailureManager.java:
##########
@@ -119,7 +137,7 @@ public FailureContext failureContext() {
* Processes failure accordingly to configured {@link FailureHandler}.
*
* @param failureCtx Failure context.
- * @return {@code True} If this very call led to Ignite node invalidation.
+ * @return {@code true} If this call leads to Ignite node invalidation and {@code false} otherwise.
Review Comment:
Ok. that makes sense, I will remove it.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893855193
##########
modules/catalog-compaction/src/integrationTest/java/org/apache/ignite/internal/catalog/compaction/ItCatalogCompactionTest.java:
##########
@@ -98,7 +98,8 @@ protected String getNodeBootstrapConfigTemplate() {
+ " },\n"
+ " clientConnector.port: {},\n"
+ " rest.port: {},\n"
- + " compute.threadPoolSize: 1\n"
+ + " compute.threadPoolSize: 1,\n"
+ + " failureHandler.dumpThreadsOnFailure: false\n"
Review Comment:
I did this based on Ignite 2's experience, but I think it can be changed in the future. The first step is using `stopNode` failure handler by default instead of `noop` [https://issues.apache.org/jira/browse/IGNITE-21456] For now, there are a lot of `green` tests on TeamCity that actually trigger failure handler, and so it will lead to huge log files that will not be analyzed (test are still `green`...)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893914232
##########
modules/core/src/main/java/org/apache/ignite/internal/util/ExceptionUtils.java:
##########
@@ -332,6 +332,52 @@ public static String getFullStackTrace(Throwable throwable) {
return sw.getBuffer().toString();
}
+ /**
+ * Checks if passed in {@code 'Throwable'} has given class in {@code 'cause'} hierarchy
+ * <b>including</b> that throwable itself.
+ * 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.
+ */
+ public static boolean hasCause(@Nullable Throwable t, @Nullable String msg, Class<?> @Nullable... cls) {
Review Comment:
>Do you mean something like as follows https://github.com/apache/ignite-3/blob/main/modules/api/src/main/java/org/apache/ignite/lang/util/TraceIdUtils.java#L42 ?
I thought I saw it elsewhere, but I cannot find that other place. Yes, the idea is reflected here well.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893955608
##########
modules/catalog-compaction/src/integrationTest/java/org/apache/ignite/internal/catalog/compaction/ItCatalogCompactionTest.java:
##########
@@ -98,7 +98,8 @@ protected String getNodeBootstrapConfigTemplate() {
+ " },\n"
+ " clientConnector.port: {},\n"
+ " rest.port: {},\n"
- + " compute.threadPoolSize: 1\n"
+ + " compute.threadPoolSize: 1,\n"
+ + " failureHandler.dumpThreadsOnFailure: false\n"
Review Comment:
IMHO, we need to change the default value to `stopNode` policy, it makes sense for production deployments.
On the other hand, our tests may use another approach as you suggested. Let's discuss this topic with Yuriy.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 merged PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "sk0x50 (via GitHub)" <gi...@apache.org>.
sk0x50 commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893844186
##########
modules/failure-handler/src/test/java/org/apache/ignite/internal/failure/FailureProcessorThreadDumpThrottlingTest.java:
##########
@@ -0,0 +1,232 @@
+/*
+ * 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.ignite.internal.failure;
+
+import static org.apache.ignite.internal.failure.FailureType.SYSTEM_CRITICAL_OPERATION_TIMEOUT;
+import static org.apache.ignite.internal.failure.FailureType.SYSTEM_WORKER_BLOCKED;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willSucceedFast;
+import static org.apache.ignite.internal.util.IgniteUtils.THREAD_DUMP_MSG;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.Consumer;
+import org.apache.ignite.internal.configuration.testframework.ConfigurationExtension;
+import org.apache.ignite.internal.configuration.testframework.InjectConfiguration;
+import org.apache.ignite.internal.failure.configuration.FailureProcessorConfiguration;
+import org.apache.ignite.internal.manager.ComponentContext;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.apache.ignite.internal.testframework.log4j2.LogInspector;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+/**
+ * Tests for {@link FailureProcessor} thread dump throttling.
+ */
+@ExtendWith(ConfigurationExtension.class)
+public class FailureProcessorThreadDumpThrottlingTest extends BaseIgniteAbstractTest {
+ @InjectConfiguration("mock: { "
+ + "oomBufferSizeBites=0, "
+ + "dumpThreadsOnFailure=true,"
+ + "dumpThreadsThrottlingTimeoutMillis=0,"
+ + "handler {type=noop} }")
+ private static FailureProcessorConfiguration enabledThreadDumpNoThrottlingConfiguration;
+
+ @InjectConfiguration("mock: { "
+ + "oomBufferSizeBites=0, "
+ + "dumpThreadsOnFailure=true,"
+ + "dumpThreadsThrottlingTimeoutMillis=3000,"
+ + "handler {type=noop} }")
+ private static FailureProcessorConfiguration enabledThreadDumpWithThrottlingConfiguration;
+
+ @InjectConfiguration("mock: { "
+ + "oomBufferSizeBites=0, "
+ + "dumpThreadsOnFailure=false,"
+ + "dumpThreadsThrottlingTimeoutMillis=0,"
+ + "handler {type=noop} }")
+ private static FailureProcessorConfiguration disabledThreadDumpConfiguration;
+
+ /**
+ * Tests that thread dumps will not get if 'dumpThreadsOnFailure' equals to `false`.
+ */
+ @Test
+ public void testNoThreadDumps() {
+ LogInspector logInspector = new LogInspector(
+ FailureManager.class.getName(),
+ evt -> evt.getMessage().getFormattedMessage().startsWith(THREAD_DUMP_MSG));
+
+ logInspector.start();
+ try {
+ FailureManager failureManager = new FailureManager(() -> {}, disabledThreadDumpConfiguration);
+
+ try {
+ assertThat(failureManager.startAsync(new ComponentContext()), willSucceedFast());
+
+ FailureContext failureCtx = new FailureContext(SYSTEM_WORKER_BLOCKED, new Throwable("Failure context error"));
+
+ assertThat(failureManager.process(failureCtx), is(false));
+ } finally {
+ assertThat(failureManager.stopAsync(new ComponentContext()), willSucceedFast());
+ }
+ } finally {
+ logInspector.stop();
+ }
+
+ assertThat(logInspector.isMatched(), is(false));
+ }
+
+ /**
+ * Tests that thread dumps will get for every failure for disabled throttling.
+ */
+ @Test
+ public void testNoThrottling() {
+ AtomicInteger messageCounter = new AtomicInteger();
+
+ LogInspector logInspector = new LogInspector(
+ FailureManager.class.getName(),
+ evt -> evt.getMessage().getFormattedMessage().startsWith(THREAD_DUMP_MSG),
+ messageCounter::incrementAndGet);
+
+ logInspector.start();
+ try {
+ testFailureProcessing(enabledThreadDumpNoThrottlingConfiguration, failureProcessor -> {
+ FailureContext failureCtx = new FailureContext(SYSTEM_WORKER_BLOCKED, new Throwable("Failure context error"));
+
+ // Trigger the failure processing twice.
+ assertThat(failureProcessor.process(failureCtx), is(false));
+
+ assertThat(failureProcessor.process(failureCtx), is(false));
+ });
+ } finally {
+ logInspector.stop();
+ }
+
+ assertThat(messageCounter.get(), is(2));
+ }
+
+ /**
+ * Tests that thread dumps will be throttled and will be generated again after timeout exceeded.
+ */
+ @Test
+ public void testThrottling() {
+ AtomicInteger threadDumpMessageCounter = new AtomicInteger();
+ AtomicInteger throttlingMessageCounter = new AtomicInteger();
+
+ LogInspector logInspector = new LogInspector(FailureManager.class.getName());
+
+ logInspector.addHandler(
+ evt -> evt.getMessage().getFormattedMessage().startsWith(THREAD_DUMP_MSG),
+ threadDumpMessageCounter::incrementAndGet);
+ logInspector.addHandler(
+ evt -> evt.getMessage().getFormattedMessage().startsWith("Thread dump is hidden due to throttling settings."),
+ throttlingMessageCounter::incrementAndGet);
+
+ logInspector.start();
+ try {
+ testFailureProcessing(enabledThreadDumpWithThrottlingConfiguration, failureProcessor -> {
+ FailureContext failureCtx = new FailureContext(SYSTEM_WORKER_BLOCKED, new Throwable("Failure context error"));
+
+ // Trigger the failure processing twice.
+ for (int i = 0; i < 2; i++) {
+ assertThat(failureProcessor.process(failureCtx), is(false));
+ }
+
+ try {
+ Thread.sleep(3000);
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+
+ // Trigger the failure processing twice.
+ for (int i = 0; i < 2; i++) {
+ assertThat(failureProcessor.process(failureCtx), is(false));
+ }
+ });
+ } finally {
+ logInspector.stop();
+ }
+
+ assertThat(threadDumpMessageCounter.get(), is(2));
+ assertThat(throttlingMessageCounter.get(), is(2));
+ }
+
+ /**
+ * Tests that thread dumps will be throttled per failure type and will be generated again after timeout exceeded.
+ */
+ @Test
+ public void testThrottlingPerFailureType() {
+ AtomicInteger threadDumpMessageCounter = new AtomicInteger();
+ AtomicInteger throttlingMessageCounter = new AtomicInteger();
+
+ LogInspector logInspector = new LogInspector(FailureManager.class.getName());
+
+ logInspector.addHandler(
+ evt -> evt.getMessage().getFormattedMessage().startsWith(THREAD_DUMP_MSG),
+ threadDumpMessageCounter::incrementAndGet);
+ logInspector.addHandler(
+ evt -> evt.getMessage().getFormattedMessage().startsWith("Thread dump is hidden due to throttling settings."),
+ throttlingMessageCounter::incrementAndGet);
+
+ logInspector.start();
+ try {
+ testFailureProcessing(enabledThreadDumpWithThrottlingConfiguration, failureProcessor -> {
+ FailureContext workerBlockedFailureCtx =
+ new FailureContext(SYSTEM_WORKER_BLOCKED, new Throwable("Failure context error"));
+
+ FailureContext opTimeoutFailureCtx =
+ new FailureContext(SYSTEM_CRITICAL_OPERATION_TIMEOUT, new Throwable("Failure context error"));
+
+ for (int i = 0; i < 2; i++) {
+ failureProcessor.process(workerBlockedFailureCtx);
+ failureProcessor.process(opTimeoutFailureCtx);
+ }
+
+ try {
+ Thread.sleep(3000);
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+
+ for (int i = 0; i < 2; i++) {
+ failureProcessor.process(workerBlockedFailureCtx);
+ failureProcessor.process(opTimeoutFailureCtx);
+ }
+ });
+ } finally {
+ logInspector.stop();
+ }
+
+ assertThat(threadDumpMessageCounter.get(), is(4));
+ assertThat(throttlingMessageCounter.get(), is(4));
+ }
+
+ /**
+ * Creates a new instance of {@link FailureManager} with the given configuration and runs the test represented by {@code test} closure.
+ */
+ public static void testFailureProcessing(FailureProcessorConfiguration configuration, Consumer<FailureProcessor> test) {
Review Comment:
Yep, it can be package-private. Changed
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20449 Add support generating thread dump on failure [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4923:
URL: https://github.com/apache/ignite-3/pull/4923#discussion_r1893916060
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/impl/AbstractClientService.java:
##########
@@ -233,7 +233,7 @@ public void complete(final Object result, final Throwable err) {
}
}
else {
- if (ThrowUtil.hasCause(err, null, PeerUnavailableException.class, ConnectException.class))
Review Comment:
No, I had no idea it was our code originally. I revoke my suggestion :)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org