You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "csudharsanan (via GitHub)" <gi...@apache.org> on 2023/11/08 03:30:06 UTC
[PR] Handling throwable during Helix message processing [helix]
csudharsanan opened a new pull request, #2696:
URL: https://github.com/apache/helix/pull/2696
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
fixes #2695
### Description
- [ ] When we encounter any Throwable Error like OOME, we do not handle it currently. We should ideally cleanup ZK state (remove the message) and allow Helix to continue working on failed task. Clean-up and retry code path should be executed on encountering Throwable of any type.
### Tests
- [ ] TestHelixTaskExecutor:
- testThrowableHandlingOnMessageProcess
```
[INFO] Reactor Summary for Apache Helix 1.3.2-SNAPSHOT:
[INFO]
[INFO] Apache Helix ....................................... SUCCESS [ 1.793 s]
[INFO] Apache Helix :: Metrics Common ..................... SUCCESS [ 2.393 s]
[INFO] Apache Helix :: Metadata Store Directory Common .... SUCCESS [ 1.479 s]
[INFO] Apache Helix :: ZooKeeper API ...................... SUCCESS [ 3.975 s]
[INFO] Apache Helix :: Helix Common ....................... SUCCESS [ 1.533 s]
[INFO] Apache Helix :: Core ............................... SUCCESS [ 24.984 s]
[INFO] Apache Helix :: Admin Webapp ....................... SUCCESS [ 2.978 s]
[INFO] Apache Helix :: Restful Interface .................. SUCCESS [ 5.904 s]
[INFO] Apache Helix :: Distributed Lock ................... SUCCESS [ 1.160 s]
[INFO] Apache Helix :: HelixAgent ......................... SUCCESS [ 1.959 s]
[INFO] Apache Helix :: Front End .......................... SUCCESS [03:20 min]
[INFO] Apache Helix :: Recipes ............................ SUCCESS [ 0.031 s]
[INFO] Apache Helix :: Recipes :: Rabbitmq Consumer Group . SUCCESS [ 1.484 s]
[INFO] Apache Helix :: Recipes :: Rsync Replicated File Store SUCCESS [ 1.493 s]
[INFO] Apache Helix :: Recipes :: distributed lock manager SUCCESS [ 1.121 s]
[INFO] Apache Helix :: Recipes :: distributed task execution SUCCESS [ 1.137 s]
[INFO] Apache Helix :: Recipes :: service discovery ....... SUCCESS [ 1.030 s]
[INFO] Apache Helix :: View Aggregator .................... SUCCESS [ 2.253 s]
[INFO] Apache Helix :: Meta Client ........................ SUCCESS [ 2.138 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 04:20 min
[INFO] Finished at: 2023-11-07T19:21:16-08:00
[INFO] ------------------------------------------------------------------------
```
```
[INFO] Tests run: 20, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 47.314 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 20, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO]
[INFO] --- jacoco:0.8.6:report (generate-code-coverage-report) @ helix-core ---
[INFO] Loading execution data file /Users/csudhars/Documents/GitHub/csudharsanan_helix/helix/helix-core/target/jacoco.exec
[INFO] Analyzed bundle 'Apache Helix :: Core' with 947 classes
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:04 min
[INFO] Finished at: 2023-11-07T19:24:28-08:00
[INFO] ------------------------------------------------------------------------
```
- The following is the result of the "mvn test" command on the appropriate module:
(If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)
### Changes that Break Backward Compatibility (Optional)
- My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:
(Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "csudharsanan (via GitHub)" <gi...@apache.org>.
csudharsanan commented on code in PR #2696:
URL: https://github.com/apache/helix/pull/2696#discussion_r1420450803
##########
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java:
##########
@@ -488,11 +490,11 @@ public boolean scheduleTask(MessageTask task) {
"Message handling task already sheduled for " + taskId, manager);
}
}
- } catch (Exception e) {
+ } catch (Throwable e) {
Review Comment:
updated variable name to t. Since OOME encountered in this GCN is an error, we had to update this to throwable instead of Exception.
OOME has following lineage:
> java.lang.Throwable
-->java.lang.Error
-->java.lang.VirtualMachineError
--->java.lang.OutOfMemoryError
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "csudharsanan (via GitHub)" <gi...@apache.org>.
csudharsanan commented on PR #2696:
URL: https://github.com/apache/helix/pull/2696#issuecomment-1815009217
This PR is ready to be merged
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2696:
URL: https://github.com/apache/helix/pull/2696#discussion_r1396171964
##########
helix-core/src/test/java/org/apache/helix/messaging/handling/TestHelixTaskExecutor2.java:
##########
@@ -0,0 +1,132 @@
+package org.apache.helix.messaging.handling;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.NotificationContext;
+import org.apache.helix.PropertyKey.Builder;
+import org.apache.helix.mock.MockManager;
+import org.apache.helix.mock.participant.MockHelixTaskExecutor;
+import org.apache.helix.mock.statemodel.MockMasterSlaveStateModel;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.Message;
+import org.apache.helix.model.Message.MessageType;
+import org.apache.helix.model.StateModelDefinition;
+import org.apache.helix.participant.statemachine.StateModelFactory;
+import org.apache.helix.tools.StateModelConfigGenerator;
+import org.mockito.MockitoAnnotations;
+import org.mockito.Spy;
+import org.testng.AssertJUnit;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doThrow;
+
+public class TestHelixTaskExecutor2 {
Review Comment:
The naming is a bit wired. Please considering having a more meaningful one.
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "desaikomal (via GitHub)" <gi...@apache.org>.
desaikomal commented on code in PR #2696:
URL: https://github.com/apache/helix/pull/2696#discussion_r1387001998
##########
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java:
##########
@@ -276,7 +276,7 @@ private void startMonitorThread() {
/** Dedicated Thread pool can be provided in configuration or by client.
* This method is to check it and update the thread pool if necessary.
*/
- private void updateStateTransitionMessageThreadPool(Message message, HelixManager manager) {
+ public void updateStateTransitionMessageThreadPool(Message message, HelixManager manager) {
Review Comment:
there is an annotation so that only it is visible for Test only.
can we use that, instead of making it 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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "csudharsanan (via GitHub)" <gi...@apache.org>.
csudharsanan commented on code in PR #2696:
URL: https://github.com/apache/helix/pull/2696#discussion_r1420446339
##########
helix-core/src/main/java/org/apache/helix/util/StatusUpdateUtil.java:
##########
@@ -406,6 +406,15 @@ public void logError(Message message, Class classInfo, Exception e, String addit
additionalInfo + sw.toString(), manager);
}
+ public void logError(Message message, Class classInfo, Throwable t, String additionalInfo,
Review Comment:
makes sense, 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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2696:
URL: https://github.com/apache/helix/pull/2696#discussion_r1446415403
##########
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java:
##########
@@ -488,10 +490,10 @@ public boolean scheduleTask(MessageTask task) {
"Message handling task already sheduled for " + taskId, manager);
}
}
- } catch (Exception e) {
- LOG.error("Error while executing task. " + message, e);
+ } catch (Throwable t) {
+ LOG.error("Error while executing task. " + message, t);
_statusUpdateUtil
- .logError(message, HelixTaskExecutor.class, e, "Error while executing task " + e,
+ .logError(message, HelixTaskExecutor.class, "Error while executing task " + t,
Review Comment:
Please consider not integrate the Throwable in message. If we do it this way, it only print out a short description of this throwable.
We may want to print the stack trace and other information. Please consider something like.
```
public void logError(Message message, Class classInfo, Exception e, String additionalInfo,
HelixManager manager)
```
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "himanshukandwal (via GitHub)" <gi...@apache.org>.
himanshukandwal commented on code in PR #2696:
URL: https://github.com/apache/helix/pull/2696#discussion_r1387086636
##########
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java:
##########
@@ -276,7 +276,7 @@ private void startMonitorThread() {
/** Dedicated Thread pool can be provided in configuration or by client.
* This method is to check it and update the thread pool if necessary.
*/
- private void updateStateTransitionMessageThreadPool(Message message, HelixManager manager) {
+ public void updateStateTransitionMessageThreadPool(Message message, HelixManager manager) {
Review Comment:
Yes, pls use @VisibleForTesting from Guava
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "csudharsanan (via GitHub)" <gi...@apache.org>.
csudharsanan commented on code in PR #2696:
URL: https://github.com/apache/helix/pull/2696#discussion_r1387195493
##########
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java:
##########
@@ -276,7 +276,7 @@ private void startMonitorThread() {
/** Dedicated Thread pool can be provided in configuration or by client.
* This method is to check it and update the thread pool if necessary.
*/
- private void updateStateTransitionMessageThreadPool(Message message, HelixManager manager) {
+ public void updateStateTransitionMessageThreadPool(Message message, HelixManager manager) {
Review Comment:
Had to move the TestClass from **org.apache.helix** Package to **org.apache.helix.messaging.handling** Package to use annotations. Since there was another test class with the same name **TestHelixTaskExecutor** in the package : org.apache.helix.messaging.handling, I ended up renaming this one to **TestHelixTaskExecutor2**.
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
junkaixue commented on code in PR #2696:
URL: https://github.com/apache/helix/pull/2696#discussion_r1419689225
##########
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java:
##########
@@ -488,11 +490,11 @@ public boolean scheduleTask(MessageTask task) {
"Message handling task already sheduled for " + taskId, manager);
}
}
- } catch (Exception e) {
+ } catch (Throwable e) {
Review Comment:
In Helix, we use general Exception instead of Throwable. Even Throwable, you need to replace variable name to t.
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "csudharsanan (via GitHub)" <gi...@apache.org>.
csudharsanan commented on code in PR #2696:
URL: https://github.com/apache/helix/pull/2696#discussion_r1387190430
##########
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java:
##########
@@ -488,11 +488,17 @@ public boolean scheduleTask(MessageTask task) {
"Message handling task already sheduled for " + taskId, manager);
}
}
- } catch (Exception e) {
- LOG.error("Error while executing task. " + message, e);
- _statusUpdateUtil
- .logError(message, HelixTaskExecutor.class, e, "Error while executing task " + e,
- manager);
+ } catch (Throwable e) {
+ String errorMessage = "Error while executing task. " + message;
+ LOG.error(errorMessage, e);
Review Comment:
thanks @xyuanlu , fixed this.
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2696:
URL: https://github.com/apache/helix/pull/2696#discussion_r1387007983
##########
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java:
##########
@@ -488,11 +488,17 @@ public boolean scheduleTask(MessageTask task) {
"Message handling task already sheduled for " + taskId, manager);
}
}
- } catch (Exception e) {
- LOG.error("Error while executing task. " + message, e);
- _statusUpdateUtil
- .logError(message, HelixTaskExecutor.class, e, "Error while executing task " + e,
- manager);
+ } catch (Throwable e) {
+ String errorMessage = "Error while executing task. " + message;
+ LOG.error(errorMessage, e);
Review Comment:
Please consider using standard log message consolidation. It will safe new string object creation.
Log.error("xxxxx {} " , var);
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "himanshukandwal (via GitHub)" <gi...@apache.org>.
himanshukandwal commented on code in PR #2696:
URL: https://github.com/apache/helix/pull/2696#discussion_r1446672976
##########
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java:
##########
@@ -488,10 +490,10 @@ public boolean scheduleTask(MessageTask task) {
"Message handling task already sheduled for " + taskId, manager);
}
}
- } catch (Exception e) {
- LOG.error("Error while executing task. " + message, e);
+ } catch (Throwable t) {
+ LOG.error("Error while executing task. " + message, t);
_statusUpdateUtil
- .logError(message, HelixTaskExecutor.class, e, "Error while executing task " + e,
+ .logError(message, HelixTaskExecutor.class, "Error while executing task " + t,
Review Comment:
Can close this one, as created https://github.com/apache/helix/pull/2730 for addressing the comments
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "desaikomal (via GitHub)" <gi...@apache.org>.
desaikomal commented on code in PR #2696:
URL: https://github.com/apache/helix/pull/2696#discussion_r1412266692
##########
helix-core/src/main/java/org/apache/helix/util/StatusUpdateUtil.java:
##########
@@ -406,6 +406,15 @@ public void logError(Message message, Class classInfo, Exception e, String addit
additionalInfo + sw.toString(), manager);
}
+ public void logError(Message message, Class classInfo, Throwable t, String additionalInfo,
Review Comment:
this may not be required. let us use existing one where the additional info as 'T' info.
##########
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java:
##########
@@ -488,11 +490,11 @@ public boolean scheduleTask(MessageTask task) {
"Message handling task already sheduled for " + taskId, manager);
}
}
- } catch (Exception e) {
+ } catch (Throwable e) {
LOG.error("Error while executing task. " + message, e);
_statusUpdateUtil
- .logError(message, HelixTaskExecutor.class, e, "Error while executing task " + e,
- manager);
+ .logError(message, HelixTaskExecutor.class, e, "Error while executing task. " + e,
Review Comment:
wondering if we can use the existing .logError as you are already printing 'e' as part of the additionalInfo.
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "csudharsanan (via GitHub)" <gi...@apache.org>.
csudharsanan commented on code in PR #2696:
URL: https://github.com/apache/helix/pull/2696#discussion_r1387190162
##########
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java:
##########
@@ -276,7 +276,7 @@ private void startMonitorThread() {
/** Dedicated Thread pool can be provided in configuration or by client.
* This method is to check it and update the thread pool if necessary.
*/
- private void updateStateTransitionMessageThreadPool(Message message, HelixManager manager) {
+ public void updateStateTransitionMessageThreadPool(Message message, HelixManager manager) {
Review Comment:
thanks for the review @desaikomal and @himanshukandwal , fixed 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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
junkaixue closed pull request #2696: Handling throwable during Helix message processing
URL: https://github.com/apache/helix/pull/2696
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "csudharsanan (via GitHub)" <gi...@apache.org>.
csudharsanan commented on code in PR #2696:
URL: https://github.com/apache/helix/pull/2696#discussion_r1396349813
##########
helix-core/src/test/java/org/apache/helix/messaging/handling/TestHelixTaskExecutor2.java:
##########
@@ -0,0 +1,132 @@
+package org.apache.helix.messaging.handling;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.HelixDataAccessor;
+import org.apache.helix.NotificationContext;
+import org.apache.helix.PropertyKey.Builder;
+import org.apache.helix.mock.MockManager;
+import org.apache.helix.mock.participant.MockHelixTaskExecutor;
+import org.apache.helix.mock.statemodel.MockMasterSlaveStateModel;
+import org.apache.helix.model.CurrentState;
+import org.apache.helix.model.Message;
+import org.apache.helix.model.Message.MessageType;
+import org.apache.helix.model.StateModelDefinition;
+import org.apache.helix.participant.statemachine.StateModelFactory;
+import org.apache.helix.tools.StateModelConfigGenerator;
+import org.mockito.MockitoAnnotations;
+import org.mockito.Spy;
+import org.testng.AssertJUnit;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doThrow;
+
+public class TestHelixTaskExecutor2 {
Review Comment:
Renamed to TestCMTaskExecutor as there is another TestHelixTaskExecutor under the same package.
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "csudharsanan (via GitHub)" <gi...@apache.org>.
csudharsanan commented on PR #2696:
URL: https://github.com/apache/helix/pull/2696#issuecomment-1815031078
> Thanks @csudharsanan for composing the PR! Normally we require a "approval" before merge. We will take another round of review :D
> Thanks @csudharsanan for composing the PR! Normally we require a "approval" before merge. We will take another round of review :D
Ahh my bad.
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "csudharsanan (via GitHub)" <gi...@apache.org>.
csudharsanan commented on code in PR #2696:
URL: https://github.com/apache/helix/pull/2696#discussion_r1387195493
##########
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java:
##########
@@ -276,7 +276,7 @@ private void startMonitorThread() {
/** Dedicated Thread pool can be provided in configuration or by client.
* This method is to check it and update the thread pool if necessary.
*/
- private void updateStateTransitionMessageThreadPool(Message message, HelixManager manager) {
+ public void updateStateTransitionMessageThreadPool(Message message, HelixManager manager) {
Review Comment:
Had to move the TestClass from **org.apache.helix** Package to **org.apache.helix.messaging.handling** Package to use annotations. Since there was another test class with the same name **TestHelixExecutor** in the package : org.apache.helix.messaging.handling, I ended up renaming this one to **TestHelixExecutor2**.
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on PR #2696:
URL: https://github.com/apache/helix/pull/2696#issuecomment-1815014672
Thanks @csudharsanan for composing the PR! Normally we require a "approval" before merge. We will take another round of review :D
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "csudharsanan (via GitHub)" <gi...@apache.org>.
csudharsanan commented on code in PR #2696:
URL: https://github.com/apache/helix/pull/2696#discussion_r1420447061
##########
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java:
##########
@@ -488,11 +490,11 @@ public boolean scheduleTask(MessageTask task) {
"Message handling task already sheduled for " + taskId, manager);
}
}
- } catch (Exception e) {
+ } catch (Throwable e) {
LOG.error("Error while executing task. " + message, e);
_statusUpdateUtil
- .logError(message, HelixTaskExecutor.class, e, "Error while executing task " + e,
- manager);
+ .logError(message, HelixTaskExecutor.class, e, "Error while executing task. " + e,
Review Comment:
fixed this as part of other 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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Handling throwable during Helix message processing [helix]
Posted by "csudharsanan (via GitHub)" <gi...@apache.org>.
csudharsanan commented on PR #2696:
URL: https://github.com/apache/helix/pull/2696#issuecomment-1847161785
> btw what is CM in TestCM ??
CM stands for ClusterManager, there were two TestHelixTaskExecutors within the same module and had to rename the new Test class to this.
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org