You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "rpuch (via GitHub)" <gi...@apache.org> on 2025/04/04 14:18:33 UTC
[PR] IGNITE-24996 Notify failure handler instead of logging errors [ignite-3]
rpuch opened a new pull request, #5574:
URL: https://github.com/apache/ignite-3/pull/5574
https://issues.apache.org/jira/browse/IGNITE-24996
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2030723540
##########
modules/transactions/build.gradle:
##########
@@ -45,6 +45,7 @@ dependencies {
implementation project(':ignite-workers')
implementation project(':ignite-low-watermark')
implementation project(':ignite-system-view-api')
+ implementation project(':ignite-failure-handler')
Review Comment:
Which one is in the public API of this module? `FailureProcessor`? If yes, then disregard my comment.
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2030943166
##########
modules/core/src/main/java/org/apache/ignite/internal/failure/FailureProcessorUtils.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.CRITICAL_ERROR;
+import static org.apache.ignite.lang.ErrorGroups.Common.INTERNAL_ERR;
+
+import org.apache.ignite.internal.lang.IgniteInternalException;
+
+/**
+ * Utils making it easier to report failures with {@link FailureProcessor}.
+ */
+public class FailureProcessorUtils {
+ /**
+ * Reports a failure to a failure processor capturing the call site context.
+ *
+ * @param processor Processor used to process the failure.
+ * @param th The failure.
+ * @param messageFormat Message format (same as {@link String#format(String, Object...)} accepts).
+ * @param args Arguments for the message.
+ */
+ public static void processCriticalFailure(FailureProcessor processor, Throwable th, String messageFormat, Object... args) {
+ processor.process(new FailureContext(CRITICAL_ERROR,
+ new IgniteInternalException(INTERNAL_ERR, String.format(messageFormat, args))
Review Comment:
What's 'our own formatter'?
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2031315984
##########
modules/core/src/main/java/org/apache/ignite/internal/failure/FailureProcessorUtils.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.CRITICAL_ERROR;
+import static org.apache.ignite.lang.ErrorGroups.Common.INTERNAL_ERR;
+
+import org.apache.ignite.internal.lang.IgniteInternalException;
+
+/**
+ * Utils making it easier to report failures with {@link FailureProcessor}.
+ */
+public class FailureProcessorUtils {
+ /**
+ * Reports a failure to a failure processor capturing the call site context.
+ *
+ * @param processor Processor used to process the failure.
+ * @param th The failure.
+ * @param messageFormat Message format (same as {@link String#format(String, Object...)} accepts).
+ * @param args Arguments for the message.
+ */
+ public static void processCriticalFailure(FailureProcessor processor, Throwable th, String messageFormat, Object... args) {
+ processor.process(new FailureContext(CRITICAL_ERROR,
+ new IgniteInternalException(INTERNAL_ERR, String.format(messageFormat, args))
Review Comment:
Error message is now constructed externally to failure handling machinery. In some places where it's still used I left `String.format()` as it is used widely in the project. There doesn't seem to be any restriction on its use, despite having `IgniteStringFormatter`.
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2031549316
##########
modules/core/src/main/java/org/apache/ignite/internal/hlc/HybridClockImpl.java:
##########
@@ -133,7 +148,12 @@ private void notifyUpdateListeners(long newTs) {
try {
listener.onUpdate(newTs);
} catch (Throwable e) {
- log.error("ClockUpdateListener#onUpdate() failed for {} at {}", e, listener, newTs);
+ if (failureProcessor != null) {
+ String errorMessage = String.format("ClockUpdateListener#onUpdate() failed for %s at %s", listener, newTs);
+ failureProcessor.process(new FailureContext(e, errorMessage));
+ } else {
+ log.error("ClockUpdateListener#onUpdate() failed for {} at {}", e, listener, newTs);
+ }
Review Comment:
Fixed
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2030700826
##########
modules/core/src/main/java/org/apache/ignite/internal/hlc/HybridClockImpl.java:
##########
@@ -44,6 +49,14 @@ public class HybridClockImpl implements HybridClock {
private final List<ClockUpdateListener> updateListeners = new CopyOnWriteArrayList<>();
+ public HybridClockImpl() {
Review Comment:
Fair enough, should we mark it as TestOnly?
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2031446231
##########
modules/core/src/main/java/org/apache/ignite/internal/failure/FailureProcessorUtils.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.CRITICAL_ERROR;
+import static org.apache.ignite.lang.ErrorGroups.Common.INTERNAL_ERR;
+
+import org.apache.ignite.internal.lang.IgniteInternalException;
+
+/**
+ * Utils making it easier to report failures with {@link FailureProcessor}.
+ */
+public class FailureProcessorUtils {
+ /**
+ * Reports a failure to a failure processor capturing the call site context.
+ *
+ * @param processor Processor used to process the failure.
+ * @param th The failure.
+ * @param messageFormat Message format (same as {@link String#format(String, Object...)} accepts).
+ * @param args Arguments for the message.
+ */
+ public static void processCriticalFailure(FailureProcessor processor, Throwable th, String messageFormat, Object... args) {
+ processor.process(new FailureContext(CRITICAL_ERROR,
+ new IgniteInternalException(INTERNAL_ERR, String.format(messageFormat, args))
Review Comment:
If we will count all changes my guts telling me that 1/4 of 1,227 changes in the code itself will be just changing {} to %s. You could've avoid it by using IgniteStringFormatter instead.
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2030948603
##########
modules/core/src/main/java/org/apache/ignite/internal/failure/FailureProcessorUtils.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.CRITICAL_ERROR;
+import static org.apache.ignite.lang.ErrorGroups.Common.INTERNAL_ERR;
+
+import org.apache.ignite.internal.lang.IgniteInternalException;
+
+/**
+ * Utils making it easier to report failures with {@link FailureProcessor}.
+ */
+public class FailureProcessorUtils {
+ /**
+ * Reports a failure to a failure processor capturing the call site context.
+ *
+ * @param processor Processor used to process the failure.
+ * @param th The failure.
+ * @param messageFormat Message format (same as {@link String#format(String, Object...)} accepts).
+ * @param args Arguments for the message.
+ */
+ public static void processCriticalFailure(FailureProcessor processor, Throwable th, String messageFormat, Object... args) {
+ processor.process(new FailureContext(CRITICAL_ERROR,
+ new IgniteInternalException(INTERNAL_ERR, String.format(messageFormat, args))
Review Comment:
IgniteStringFormatter which we are using inside logger. Using this formatter will reduce the amount of code changes as 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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2030944445
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/ZoneRebalanceRaftGroupEventsListener.java:
##########
@@ -203,7 +209,12 @@ public void onLeaderElected(long term) {
}
} catch (Exception e) {
// TODO: IGNITE-14693
- LOG.warn("Unable to start rebalance [tablePartitionId, term={}]", e, zonePartitionId, term);
+ processCriticalFailure(
+ failureProcessor,
+ e,
+ "Unable to start rebalance [tablePartitionId=%s, term=%s]",
+ zonePartitionId, term
Review Comment:
I did this on purpose to group format arguments together
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2030942116
##########
modules/transactions/build.gradle:
##########
@@ -45,6 +45,7 @@ dependencies {
implementation project(':ignite-workers')
implementation project(':ignite-low-watermark')
implementation project(':ignite-system-view-api')
+ implementation project(':ignite-failure-handler')
Review Comment:
`FailureProcessor` comes from `ignite-core`. How is it related to `ignite-transactions` API?
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2030690809
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/LogicalTopologyImpl.java:
##########
@@ -53,15 +58,24 @@ public class LogicalTopologyImpl implements LogicalTopology {
private final ClusterStateStorage storage;
+ private final FailureProcessor failureProcessor;
+
private final ClusterStateStorageManager clusterStateStorageManager;
private final List<LogicalTopologyEventListener> listeners = new CopyOnWriteArrayList<>();
private volatile @Nullable UUID clusterId;
/** Constructor. */
+ @TestOnly
Review Comment:
Yes, if 13 tests are touched. I think in this case it's easier to add a simple test-only constructor instead
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2031438633
##########
modules/core/src/main/java/org/apache/ignite/internal/failure/FailureContext.java:
##########
@@ -30,15 +30,49 @@ public class FailureContext {
/** Error. */
private final Throwable err;
+ /** Message describing the failure. */
+ private final String message;
+
/**
- * Creates instance of {@link FailureContext}.
+ * Creates instance of {@link FailureContext} of {@link FailureType#CRITICAL_ERROR} type.
+ *
+ * @param err Exception.
+ */
+ public FailureContext(Throwable err) {
+ this(FailureType.CRITICAL_ERROR, err);
+ }
+
+ /**
+ * Creates instance of {@link FailureContext} corresponding to an unknown failure.
*
* @param type Failure type.
* @param err Exception.
*/
public FailureContext(FailureType type, Throwable err) {
+ this(type, err, "Unknown error");
Review Comment:
Unknown error - should be a constant
##########
modules/core/src/main/java/org/apache/ignite/internal/hlc/HybridClockImpl.java:
##########
@@ -133,7 +148,12 @@ private void notifyUpdateListeners(long newTs) {
try {
listener.onUpdate(newTs);
} catch (Throwable e) {
- log.error("ClockUpdateListener#onUpdate() failed for {} at {}", e, listener, newTs);
+ if (failureProcessor != null) {
+ String errorMessage = String.format("ClockUpdateListener#onUpdate() failed for %s at %s", listener, newTs);
+ failureProcessor.process(new FailureContext(e, errorMessage));
+ } else {
+ log.error("ClockUpdateListener#onUpdate() failed for {} at {}", e, listener, newTs);
+ }
Review Comment:
This part is screaming to use IgniteStringFormatter instead of String.format.
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2031447791
##########
modules/core/src/main/java/org/apache/ignite/internal/failure/FailureContext.java:
##########
@@ -30,15 +30,49 @@ public class FailureContext {
/** Error. */
private final Throwable err;
+ /** Message describing the failure. */
+ private final String message;
+
/**
- * Creates instance of {@link FailureContext}.
+ * Creates instance of {@link FailureContext} of {@link FailureType#CRITICAL_ERROR} type.
+ *
+ * @param err Exception.
+ */
+ public FailureContext(Throwable err) {
+ this(FailureType.CRITICAL_ERROR, err);
+ }
+
+ /**
+ * Creates instance of {@link FailureContext} corresponding to an unknown failure.
*
* @param type Failure type.
* @param err Exception.
*/
public FailureContext(FailureType type, Throwable err) {
+ this(type, err, "Unknown error");
Review Comment:
Let's minimize the volume of changes in this PR, it's already too huge. The current approach looks pretty natural, it's not something ugly that has to be fixed asap :)
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2031352703
##########
modules/failure-handler/src/main/java/org/apache/ignite/internal/failure/FailureManager.java:
##########
@@ -299,4 +302,10 @@ private boolean throttleThreadDump(FailureType type) {
return throttle;
}
+
+ private static class StackTrackCapturingException extends IgniteInternalException {
Review Comment:
Exactly, thanks. Fixed
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2030951162
##########
modules/network/src/main/java/org/apache/ignite/internal/network/DefaultMessagingService.java:
##########
@@ -435,11 +440,11 @@ private void handleMessageFromNetwork(InNetworkObject inNetworkObject) {
try {
handleStartingWithFirstHandler(payload, finalCorrelationId, inNetworkObject, firstHandlerContext, handlerContexts);
} catch (Throwable e) {
- logAndRethrowIfError(inNetworkObject, e);
+ handleAndRethrowIfError(inNetworkObject, e);
} finally {
long tookMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startedNanos);
- if (tookMillis > 100) {
+ if (tookMillis > 100 && IgniteSystemProperties.getBoolean(IgniteSystemProperties.LONG_HANDLING_LOGGING_ENABLED, false)) {
Review Comment:
It's just a little change I had to do in another PR, but forgot to do it.
Added a static import
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2031444176
##########
modules/core/src/main/java/org/apache/ignite/internal/failure/FailureContext.java:
##########
@@ -30,15 +30,49 @@ public class FailureContext {
/** Error. */
private final Throwable err;
+ /** Message describing the failure. */
+ private final String message;
+
/**
- * Creates instance of {@link FailureContext}.
+ * Creates instance of {@link FailureContext} of {@link FailureType#CRITICAL_ERROR} type.
+ *
+ * @param err Exception.
+ */
+ public FailureContext(Throwable err) {
+ this(FailureType.CRITICAL_ERROR, err);
+ }
+
+ /**
+ * Creates instance of {@link FailureContext} corresponding to an unknown failure.
*
* @param type Failure type.
* @param err Exception.
*/
public FailureContext(FailureType type, Throwable err) {
+ this(type, err, "Unknown error");
Review Comment:
Why should it be? It's not duplicated anywhere, it's just a literal, it represents itself
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2030699670
##########
modules/core/src/main/java/org/apache/ignite/internal/failure/FailureProcessorUtils.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.CRITICAL_ERROR;
+import static org.apache.ignite.lang.ErrorGroups.Common.INTERNAL_ERR;
+
+import org.apache.ignite.internal.lang.IgniteInternalException;
+
+/**
+ * Utils making it easier to report failures with {@link FailureProcessor}.
+ */
+public class FailureProcessorUtils {
+ /**
+ * Reports a failure to a failure processor capturing the call site context.
+ *
+ * @param processor Processor used to process the failure.
+ * @param th The failure.
+ * @param messageFormat Message format (same as {@link String#format(String, Object...)} accepts).
+ * @param args Arguments for the message.
+ */
+ public static void processCriticalFailure(FailureProcessor processor, Throwable th, String messageFormat, Object... args) {
Review Comment:
This is literally the most often usage pattern now. I really don't like having yet another utils class with a static method
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2030948239
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/ZoneRebalanceRaftGroupEventsListener.java:
##########
@@ -203,7 +209,12 @@ public void onLeaderElected(long term) {
}
} catch (Exception e) {
// TODO: IGNITE-14693
- LOG.warn("Unable to start rebalance [tablePartitionId, term={}]", e, zonePartitionId, term);
+ processCriticalFailure(
+ failureProcessor,
+ e,
+ "Unable to start rebalance [tablePartitionId=%s, term=%s]",
+ zonePartitionId, term
Review Comment:
It is highly inconsistent with what you've done in this PR and what we have in a code base.
Generally speaking it is a very bad idea to mix 2 formatting ideologies together.
##########
modules/core/src/main/java/org/apache/ignite/internal/failure/FailureProcessorUtils.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.CRITICAL_ERROR;
+import static org.apache.ignite.lang.ErrorGroups.Common.INTERNAL_ERR;
+
+import org.apache.ignite.internal.lang.IgniteInternalException;
+
+/**
+ * Utils making it easier to report failures with {@link FailureProcessor}.
+ */
+public class FailureProcessorUtils {
+ /**
+ * Reports a failure to a failure processor capturing the call site context.
+ *
+ * @param processor Processor used to process the failure.
+ * @param th The failure.
+ * @param messageFormat Message format (same as {@link String#format(String, Object...)} accepts).
+ * @param args Arguments for the message.
+ */
+ public static void processCriticalFailure(FailureProcessor processor, Throwable th, String messageFormat, Object... args) {
+ processor.process(new FailureContext(CRITICAL_ERROR,
+ new IgniteInternalException(INTERNAL_ERR, String.format(messageFormat, args))
Review Comment:
IgniteStringFormatter which we are using inside logger.
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2030695326
##########
modules/core/src/main/java/org/apache/ignite/internal/failure/FailureProcessorUtils.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.CRITICAL_ERROR;
+import static org.apache.ignite.lang.ErrorGroups.Common.INTERNAL_ERR;
+
+import org.apache.ignite.internal.lang.IgniteInternalException;
+
+/**
+ * Utils making it easier to report failures with {@link FailureProcessor}.
+ */
+public class FailureProcessorUtils {
+ /**
+ * Reports a failure to a failure processor capturing the call site context.
+ *
+ * @param processor Processor used to process the failure.
+ * @param th The failure.
+ * @param messageFormat Message format (same as {@link String#format(String, Object...)} accepts).
+ * @param args Arguments for the message.
+ */
+ public static void processCriticalFailure(FailureProcessor processor, Throwable th, String messageFormat, Object... args) {
Review Comment:
Agree
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2030668198
##########
modules/core/src/main/java/org/apache/ignite/internal/hlc/HybridClockImpl.java:
##########
@@ -44,6 +49,14 @@ public class HybridClockImpl implements HybridClock {
private final List<ClockUpdateListener> updateListeners = new CopyOnWriteArrayList<>();
+ public HybridClockImpl() {
Review Comment:
Why do we need two constructors?
##########
modules/rocksdb-common/src/main/java/org/apache/ignite/internal/rocksdb/flush/RocksDbFlusher.java:
##########
@@ -185,7 +186,7 @@ public void removeColumnFamily(ColumnFamilyHandle handle) {
*
* @param schedule {@code true} if {@link RocksDB#flush(FlushOptions)} should be explicitly triggerred in the near future. Please refer
* to {@link RocksDbFlusher#RocksDbFlusher(String, IgniteSpinBusyLock, ScheduledExecutorService, ExecutorService, IntSupplier,
- * LogSyncer, Runnable)} parameters description to see what's really happening in this case.
+ * LogSyncer, FailureProcessor, Runnable)} parameters description to see what's really happening in this case.
Review Comment:
It would be easier to remove these parameters from the javadoc altogether
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/ZoneRebalanceRaftGroupEventsListener.java:
##########
@@ -203,7 +209,12 @@ public void onLeaderElected(long term) {
}
} catch (Exception e) {
// TODO: IGNITE-14693
- LOG.warn("Unable to start rebalance [tablePartitionId, term={}]", e, zonePartitionId, term);
+ processCriticalFailure(
+ failureProcessor,
+ e,
+ "Unable to start rebalance [tablePartitionId=%s, term=%s]",
Review Comment:
```suggestion
"Unable to start rebalance [zonePartitionId=%s, term=%s]",
```
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/LogicalTopologyImpl.java:
##########
@@ -53,15 +58,24 @@ public class LogicalTopologyImpl implements LogicalTopology {
private final ClusterStateStorage storage;
+ private final FailureProcessor failureProcessor;
+
private final ClusterStateStorageManager clusterStateStorageManager;
private final List<LogicalTopologyEventListener> listeners = new CopyOnWriteArrayList<>();
private volatile @Nullable UUID clusterId;
/** Constructor. */
+ @TestOnly
Review Comment:
Can this be avoided?
##########
modules/transactions/build.gradle:
##########
@@ -45,6 +45,7 @@ dependencies {
implementation project(':ignite-workers')
implementation project(':ignite-low-watermark')
implementation project(':ignite-system-view-api')
+ implementation project(':ignite-failure-handler')
Review Comment:
Why isn't it in the api section?
##########
modules/core/src/main/java/org/apache/ignite/internal/failure/FailureProcessorUtils.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.CRITICAL_ERROR;
+import static org.apache.ignite.lang.ErrorGroups.Common.INTERNAL_ERR;
+
+import org.apache.ignite.internal.lang.IgniteInternalException;
+
+/**
+ * Utils making it easier to report failures with {@link FailureProcessor}.
+ */
+public class FailureProcessorUtils {
+ /**
+ * Reports a failure to a failure processor capturing the call site context.
+ *
+ * @param processor Processor used to process the failure.
+ * @param th The failure.
+ * @param messageFormat Message format (same as {@link String#format(String, Object...)} accepts).
+ * @param args Arguments for the message.
+ */
+ public static void processCriticalFailure(FailureProcessor processor, Throwable th, String messageFormat, Object... args) {
Review Comment:
Why can't this be a method of the FailureProcessor?
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2031582596
##########
modules/core/src/main/java/org/apache/ignite/internal/failure/FailureProcessorUtils.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.CRITICAL_ERROR;
+import static org.apache.ignite.lang.ErrorGroups.Common.INTERNAL_ERR;
+
+import org.apache.ignite.internal.lang.IgniteInternalException;
+
+/**
+ * Utils making it easier to report failures with {@link FailureProcessor}.
+ */
+public class FailureProcessorUtils {
+ /**
+ * Reports a failure to a failure processor capturing the call site context.
+ *
+ * @param processor Processor used to process the failure.
+ * @param th The failure.
+ * @param messageFormat Message format (same as {@link String#format(String, Object...)} accepts).
+ * @param args Arguments for the message.
+ */
+ public static void processCriticalFailure(FailureProcessor processor, Throwable th, String messageFormat, Object... args) {
+ processor.process(new FailureContext(CRITICAL_ERROR,
+ new IgniteInternalException(INTERNAL_ERR, String.format(messageFormat, args))
Review Comment:
I doubt it; at least, there is a ton of `log.error()` instances changed to `failureHandler.process(...)`
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2031312636
##########
modules/core/src/main/java/org/apache/ignite/internal/failure/FailureProcessorUtils.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.CRITICAL_ERROR;
+import static org.apache.ignite.lang.ErrorGroups.Common.INTERNAL_ERR;
+
+import org.apache.ignite.internal.lang.IgniteInternalException;
+
+/**
+ * Utils making it easier to report failures with {@link FailureProcessor}.
+ */
+public class FailureProcessorUtils {
+ /**
+ * Reports a failure to a failure processor capturing the call site context.
+ *
+ * @param processor Processor used to process the failure.
+ * @param th The failure.
+ * @param messageFormat Message format (same as {@link String#format(String, Object...)} accepts).
+ * @param args Arguments for the message.
+ */
+ public static void processCriticalFailure(FailureProcessor processor, Throwable th, String messageFormat, Object... args) {
Review Comment:
After consulting with Slava, I changed the approach like this:
1. 'Call site context' (in the form of an exception capturing the stack trace) is created by `FailureManager` itself
2. Textual description of 'what operation failed' is added to `FailureContext`
3. Default failure type (CRITICAL_ERROR) may be omitted when constructing a `FailureContext`
4. Removed the static method (along with its class)
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch merged PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2030696056
##########
modules/core/src/main/java/org/apache/ignite/internal/failure/FailureProcessorUtils.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.CRITICAL_ERROR;
+import static org.apache.ignite.lang.ErrorGroups.Common.INTERNAL_ERR;
+
+import org.apache.ignite.internal.lang.IgniteInternalException;
+
+/**
+ * Utils making it easier to report failures with {@link FailureProcessor}.
+ */
+public class FailureProcessorUtils {
+ /**
+ * Reports a failure to a failure processor capturing the call site context.
+ *
+ * @param processor Processor used to process the failure.
+ * @param th The failure.
+ * @param messageFormat Message format (same as {@link String#format(String, Object...)} accepts).
+ * @param args Arguments for the message.
+ */
+ public static void processCriticalFailure(FailureProcessor processor, Throwable th, String messageFormat, Object... args) {
Review Comment:
I didn't want to change APIs in this PR, and it's arguable that the `FailureProcessor` should have this functionality. It knows how to report failures, but I'm not sure it should care about capturing the context.
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2030705122
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java:
##########
@@ -503,7 +503,7 @@ private CompletableFuture<Void> handle(VersionedUpdate update, HybridTimestamp m
return allOf(eventFutures.toArray(CompletableFuture[]::new))
.whenComplete((ignore, err) -> {
if (err != null) {
- failureManager.process(new FailureContext(CRITICAL_ERROR,
+ failureProcessor.process(new FailureContext(CRITICAL_ERROR,
Review Comment:
Isn't it process critical error?
##########
modules/catalog/src/main/java/org/apache/ignite/internal/catalog/CatalogManagerImpl.java:
##########
@@ -280,7 +280,7 @@ private CompletableFuture<Void> initCatalog(Catalog emptyCatalog) {
return updateLog.append(new VersionedUpdate(emptyCatalog.version() + 1, 0L, entries))
.handle((result, error) -> {
if (error != null) {
- failureManager.process(new FailureContext(CRITICAL_ERROR,
+ failureProcessor.process(new FailureContext(CRITICAL_ERROR,
Review Comment:
Isn't it process critical error?
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/ZoneRebalanceRaftGroupEventsListener.java:
##########
@@ -203,7 +209,12 @@ public void onLeaderElected(long term) {
}
} catch (Exception e) {
// TODO: IGNITE-14693
- LOG.warn("Unable to start rebalance [tablePartitionId, term={}]", e, zonePartitionId, term);
+ processCriticalFailure(
+ failureProcessor,
+ e,
+ "Unable to start rebalance [tablePartitionId=%s, term=%s]",
+ zonePartitionId, term
Review Comment:
```suggestion
zonePartitionId,
term
```
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/VolatilePageMemoryMvPartitionStorage.java:
##########
@@ -290,9 +292,10 @@ private CompletableFuture<Void> startIndexMetaTreeDestruction(RenewablePartition
return destroyTree(renewableState.indexMetaTree(), null)
.whenComplete((res, e) -> {
if (e != null) {
- LOG.error(
- "Index meta tree destruction failed: [tableId={}, partitionId={}]",
+ processCriticalFailure(
+ failureProcessor,
e,
+ "Index meta tree destruction failed: [tableId=%s, partitionId=%s]",
tableStorage.getTableId(), partitionId
Review Comment:
```suggestion
tableStorage.getTableId(),
partitionId
```
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/VolatilePageMemoryMvPartitionStorage.java:
##########
@@ -256,9 +257,10 @@ private CompletableFuture<Void> startMvDataDestruction(RenewablePartitionStorage
return destroyTree(renewableState.versionChainTree(), chainKey -> destroyVersionChain((VersionChain) chainKey, renewableState))
.whenComplete((res, e) -> {
if (e != null) {
- LOG.error(
- "Version chains destruction failed: [tableId={}, partitionId={}]",
+ processCriticalFailure(
+ failureProcessor,
e,
+ "Version chains destruction failed: [tableId=%s, partitionId=%s]",
tableStorage.getTableId(), partitionId
Review Comment:
```suggestion
tableStorage.getTableId(),
partitionId
```
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/ZonePartitionReplicaListener.java:
##########
@@ -319,15 +330,16 @@ public Map<Integer, ReplicaTableProcessor> tableReplicaProcessors() {
@Override
public void onShutdown() {
replicaProcessors.forEach((tableId, listener) -> {
- try {
- listener.onShutdown();
- } catch (Throwable th) {
- LOG.error("Error during table partition listener stop for [tableId="
- + tableId + ", partitionId=" + replicationGroupId.partitionId() + "].",
- th
- );
- }
- }
- );
+ try {
+ listener.onShutdown();
+ } catch (Throwable th) {
+ processCriticalFailure(
+ failureProcessor,
+ th,
+ "Error during table partition listener stop for [tableId=%s, partitionId=%s].",
+ tableId, replicationGroupId.partitionId()
Review Comment:
```suggestion
tableId,
replicationGroupId.partitionId()
```
##########
modules/network/src/main/java/org/apache/ignite/internal/network/DefaultMessagingService.java:
##########
@@ -558,10 +563,12 @@ private static void logAndRethrowIfError(InNetworkObject obj, Throwable e) {
);
}
} else {
- LOG.error(
- "onMessage() failed while processing {} from {}",
+ processCriticalFailure(
+ failureProcessor,
e,
- LOG.isDebugEnabled() && includeSensitive() ? message : message.toStringForLightLogging(), obj.sender()
+ "onMessage() failed while processing %s from %s",
+ LOG.isDebugEnabled() && includeSensitive() ? message : message.toStringForLightLogging(),
Review Comment:
let's move into separate variable
##########
modules/core/src/main/java/org/apache/ignite/internal/failure/FailureProcessorUtils.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.CRITICAL_ERROR;
+import static org.apache.ignite.lang.ErrorGroups.Common.INTERNAL_ERR;
+
+import org.apache.ignite.internal.lang.IgniteInternalException;
+
+/**
+ * Utils making it easier to report failures with {@link FailureProcessor}.
+ */
+public class FailureProcessorUtils {
+ /**
+ * Reports a failure to a failure processor capturing the call site context.
+ *
+ * @param processor Processor used to process the failure.
+ * @param th The failure.
+ * @param messageFormat Message format (same as {@link String#format(String, Object...)} accepts).
+ * @param args Arguments for the message.
+ */
+ public static void processCriticalFailure(FailureProcessor processor, Throwable th, String messageFormat, Object... args) {
+ processor.process(new FailureContext(CRITICAL_ERROR,
+ new IgniteInternalException(INTERNAL_ERR, String.format(messageFormat, args))
Review Comment:
Why switching from our own formatter to String.format?
##########
modules/network/src/main/java/org/apache/ignite/internal/network/DefaultMessagingService.java:
##########
@@ -435,11 +440,11 @@ private void handleMessageFromNetwork(InNetworkObject inNetworkObject) {
try {
handleStartingWithFirstHandler(payload, finalCorrelationId, inNetworkObject, firstHandlerContext, handlerContexts);
} catch (Throwable e) {
- logAndRethrowIfError(inNetworkObject, e);
+ handleAndRethrowIfError(inNetworkObject, e);
} finally {
long tookMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startedNanos);
- if (tookMillis > 100) {
+ if (tookMillis > 100 && IgniteSystemProperties.getBoolean(IgniteSystemProperties.LONG_HANDLING_LOGGING_ENABLED, false)) {
Review Comment:
Why we made this change in this PR?
And even if we need it I think it is better to have static import for LONG_HANDLING_LOGGING_ENABLED
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DataNodesManager.java:
##########
@@ -1193,11 +1199,11 @@ private CompletableFuture<?> removeDataNodesKeys(int zoneId, HybridTimestamp tim
.thenApply(StatementResult::getAsBoolean)
.whenComplete((invokeResult, e) -> {
if (e != null) {
- LOG.error(
- "Failed to delete zone's dataNodes keys [zoneId = {}, timestamp = {}]",
+ processCriticalFailure(
+ failureProcessor,
e,
- zoneId,
- timestamp
+ "Failed to delete zone's dataNodes keys [zoneId = %s, timestamp = %s]",
+ zoneId, timestamp
Review Comment:
```suggestion
zoneId,
timestamp
```
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/VolatilePageMemoryTableStorage.java:
##########
@@ -176,9 +180,11 @@ CompletableFuture<Void> clearStorageAndUpdateDataStructures(AbstractPageMemoryMv
volatilePartitionStorage.destroyStructures().whenComplete((res, ex) -> {
if (ex != null) {
- LOG.error(
- "Could not destroy structures: [tableId={}, partitionId={}]",
- ex, getTableId(), volatilePartitionStorage.partitionId()
+ processCriticalFailure(
+ failureProcessor,
+ ex,
+ "Could not destroy structures: [tableId=%s, partitionId=%s]",
+ getTableId(), volatilePartitionStorage.partitionId()
Review Comment:
```suggestion
getTableId(),
volatilePartitionStorage.partitionId()
```
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/DataNodesManager.java:
##########
@@ -1129,12 +1136,11 @@ CompletableFuture<?> onZoneCreate(
.thenApply(StatementResult::getAsBoolean)
.whenComplete((invokeResult, e) -> {
if (e != null) {
- LOG.error(
- "Failed to initialize zone's dataNodes history [zoneId = {}, timestamp = {}, dataNodes = {}]",
+ processCriticalFailure(
+ failureProcessor,
e,
- zoneId,
- timestamp,
- nodeNames(dataNodes)
+ "Failed to initialize zone's dataNodes history [zoneId = %s, timestamp = %s, dataNodes = %s]",
+ zoneId, timestamp, nodeNames(dataNodes)
Review Comment:
```suggestion
zoneId,
timestamp,
nodeNames(dataNodes)
```
##########
modules/partition-replicator/src/main/java/org/apache/ignite/internal/partition/replicator/PartitionReplicaLifecycleManager.java:
##########
@@ -804,7 +821,11 @@ private CompletableFuture<List<Assignments>> writeZoneAssignmentsToMetastore(int
})
.whenComplete((realAssignments, e) -> {
if (e != null) {
- LOG.error("Couldn't get assignments from metastore for zone [zoneId={}].", e, zoneId);
+ processCriticalFailure(
+ failureProcessor,
+ e,
+ "Couldn't get assignments from metastore for zone [zoneId=%s].", zoneId
Review Comment:
```suggestion
"Couldn't get assignments from metastore for zone [zoneId=%s].",
zoneId
```
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1296,7 +1307,12 @@ private CompletableFuture<Void> startPartitionAndStartClient(
forcedAssignments
).handle((res, ex) -> {
if (ex != null) {
- LOG.warn("Unable to update raft groups on the node [tableId={}, partitionId={}]", ex, tableId, partId);
+ processCriticalFailure(
+ failureProcessor,
+ ex,
+ "Unable to update raft groups on the node [tableId=%s, partitionId=%s]",
+ tableId, partId
Review Comment:
```suggestion
tableId,
partId
```
##########
modules/index/src/main/java/org/apache/ignite/internal/index/ChangeIndexStatusTask.java:
##########
@@ -189,9 +188,11 @@ CompletableFuture<Void> start() {
// The index's table might have been dropped while we were waiting for the ability
// to switch the index status to a new state, so IndexNotFound is not a problem.
&& !(cause instanceof IndexNotFoundValidationException)) {
- LOG.error("Error starting index task: {}", throwable, indexDescriptor.id());
-
- failureManager.process(new FailureContext(FailureType.CRITICAL_ERROR, throwable));
+ processCriticalFailure(
+ failureProcessor,
+ throwable,
+ "Error starting index task: %s", indexDescriptor.id()
Review Comment:
```suggestion
"Error starting index task: %s",
indexDescriptor.id()
```
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java:
##########
@@ -1162,7 +1168,12 @@ private CompletableFuture<Void> startLocalPartitionsAndClients(
assignmentsTimestamp
).whenComplete((res, ex) -> {
if (ex != null) {
- LOG.warn("Unable to update raft groups on the node [tableId={}, partitionId={}]", ex, tableId, partId);
+ processCriticalFailure(
+ failureProcessor,
+ ex,
+ "Unable to update raft groups on the node [tableId=%s, partitionId=%s]",
+ tableId, partId
Review Comment:
```suggestion
tableId,
partId
```
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2030944445
##########
modules/distribution-zones/src/main/java/org/apache/ignite/internal/distributionzones/rebalance/ZoneRebalanceRaftGroupEventsListener.java:
##########
@@ -203,7 +209,12 @@ public void onLeaderElected(long term) {
}
} catch (Exception e) {
// TODO: IGNITE-14693
- LOG.warn("Unable to start rebalance [tablePartitionId, term={}]", e, zonePartitionId, term);
+ processCriticalFailure(
+ failureProcessor,
+ e,
+ "Unable to start rebalance [tablePartitionId=%s, term=%s]",
+ zonePartitionId, term
Review Comment:
I made this on purpose to group format arguments together
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2030711940
##########
modules/transactions/build.gradle:
##########
@@ -45,6 +45,7 @@ dependencies {
implementation project(':ignite-workers')
implementation project(':ignite-low-watermark')
implementation project(':ignite-system-view-api')
+ implementation project(':ignite-failure-handler')
Review Comment:
Why should it be there? `FailureProcessor` is in `ignite-core`, not in `ignite-failure-handler`, and why would `transactions` module provide failure handler module implementation to other modules?
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2030696838
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/LogicalTopologyImpl.java:
##########
@@ -53,15 +58,24 @@ public class LogicalTopologyImpl implements LogicalTopology {
private final ClusterStateStorage storage;
+ private final FailureProcessor failureProcessor;
+
private final ClusterStateStorageManager clusterStateStorageManager;
private final List<LogicalTopologyEventListener> listeners = new CopyOnWriteArrayList<>();
private volatile @Nullable UUID clusterId;
/** Constructor. */
+ @TestOnly
Review Comment:
But you've already touched a million tests) This constructor looks really out of place.
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2030697954
##########
modules/core/src/main/java/org/apache/ignite/internal/hlc/HybridClockImpl.java:
##########
@@ -44,6 +49,14 @@ public class HybridClockImpl implements HybridClock {
private final List<ClockUpdateListener> updateListeners = new CopyOnWriteArrayList<>();
+ public HybridClockImpl() {
Review Comment:
With one constructor, I would have to touch 152 places where this class is instantiated, but where it does not need to report any failures
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2031343758
##########
modules/failure-handler/src/main/java/org/apache/ignite/internal/failure/FailureManager.java:
##########
@@ -299,4 +302,10 @@ private boolean throttleThreadDump(FailureType type) {
return throttle;
}
+
+ private static class StackTrackCapturingException extends IgniteInternalException {
Review Comment:
Did you mean `StackTraceCapturingException`?
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2031339414
##########
modules/transactions/build.gradle:
##########
@@ -45,6 +45,7 @@ dependencies {
implementation project(':ignite-workers')
implementation project(':ignite-low-watermark')
implementation project(':ignite-system-view-api')
+ implementation project(':ignite-failure-handler')
Review Comment:
If `FailureProcessor` is part of a public constructor, then its a part of this module's API
--
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-24996 Notify failure handler instead of logging errors [ignite-3]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #5574:
URL: https://github.com/apache/ignite-3/pull/5574#discussion_r2031436233
##########
modules/core/src/main/java/org/apache/ignite/internal/failure/FailureContext.java:
##########
@@ -30,15 +30,49 @@ public class FailureContext {
/** Error. */
private final Throwable err;
+ /** Message describing the failure. */
+ private final String message;
+
/**
- * Creates instance of {@link FailureContext}.
+ * Creates instance of {@link FailureContext} of {@link FailureType#CRITICAL_ERROR} type.
+ *
+ * @param err Exception.
+ */
+ public FailureContext(Throwable err) {
+ this(FailureType.CRITICAL_ERROR, err);
+ }
+
+ /**
+ * Creates instance of {@link FailureContext} corresponding to an unknown failure.
*
* @param type Failure type.
* @param err Exception.
*/
public FailureContext(FailureType type, Throwable err) {
+ this(type, err, "Unknown error");
Review Comment:
Feels like it is better to have static factories here rather than a lot of constructors.
In that case it will be way easier to understand what is going on here. Even without javaDocs - just looking at code itself.
--
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