You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by agresch <gi...@git.apache.org> on 2018/10/01 16:13:17 UTC
[GitHub] storm pull request #2855: STORM-3240 health checks should succeed on exit co...
GitHub user agresch opened a pull request:
https://github.com/apache/storm/pull/2855
STORM-3240 health checks should succeed on exit code 0
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/agresch/storm agresch_healthcheck
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2855.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2855
----
commit 39d28387296a57835947da45f4c5c1f92eb676e6
Author: Aaron Gresch <ag...@...>
Date: 2018-10-01T16:12:28Z
STORM-3240 health checks should succeed on exit code 0
----
---
[GitHub] storm issue #2855: STORM-3240 health checks should succeed on exit code 0
Posted by agresch <gi...@git.apache.org>.
Github user agresch commented on the issue:
https://github.com/apache/storm/pull/2855
@kishorvpatil - thanks for the explanation. I do like this behavior better.
---
[GitHub] storm pull request #2855: STORM-3240 health checks should succeed on exit co...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/2855#discussion_r222100110
--- Diff: storm-server/src/main/java/org/apache/storm/healthcheck/HealthChecker.java ---
@@ -106,13 +106,13 @@ public void run() {
BufferedReader reader = new BufferedReader(new InputStreamReader(stdin));
while ((str = reader.readLine()) != null) {
if (str.startsWith("ERROR")) {
+ LOG.warn("The healthcheck process {} exited with code {}", script, process.exitValue());
return FAILED;
}
}
return SUCCESS;
--- End diff --
@agresch, if loop should always return FAIL since exit code is != 0.
Currently, there is possibility that you will get out of if loop by return code SUCCESS.
We need something similar to
```
if (process.exitValue() != 0) {
String str;
InputStream stdin = process.getInputStream();
BufferedReader reader = new BufferedReader(new InputStreamReader(stdin));
while ((str = reader.readLine()) != null) {
if (str.startsWith("ERROR")) {
return FAILED;
}
}
LOG.warn("The healthcheck process {} exited with code {}", script, process.exitValue());
return FAILED_WITH_EXIT_CODE;
}
return SUCCESS;
```
---
[GitHub] storm pull request #2855: STORM-3240 health checks should succeed on exit co...
Posted by agresch <gi...@git.apache.org>.
Github user agresch commented on a diff in the pull request:
https://github.com/apache/storm/pull/2855#discussion_r222096924
--- Diff: storm-server/src/main/java/org/apache/storm/healthcheck/HealthChecker.java ---
@@ -106,13 +106,13 @@ public void run() {
BufferedReader reader = new BufferedReader(new InputStreamReader(stdin));
while ((str = reader.readLine()) != null) {
if (str.startsWith("ERROR")) {
+ LOG.warn("The healthcheck process {} exited with code {}", script, process.exitValue());
return FAILED;
}
}
return SUCCESS;
--- End diff --
@kishorvpatil - swapped the error codes
---
[GitHub] storm issue #2855: STORM-3240 health checks should succeed on exit code 0
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2855
I am fine with making breaking changes if it is going to make everything cleaner. This was originally based off of how Hadoop does this in hopes of being able to reuse scripts, but that didn't happen, so an exit code of 0 is success and anything else is an error feels like the simplest way to do this.
Just make sure we document how this all works.
---
[GitHub] storm pull request #2855: STORM-3240 health checks should succeed on exit co...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2855
---
[GitHub] storm issue #2855: STORM-3240 health checks should succeed on exit code 0
Posted by agresch <gi...@git.apache.org>.
Github user agresch commented on the issue:
https://github.com/apache/storm/pull/2855
@revans2 - took a guess at what you wanted documented. If this isn't what you want, just let me know.
---
[GitHub] storm pull request #2855: STORM-3240 health checks should succeed on exit co...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/2855#discussion_r222091724
--- Diff: storm-server/src/main/java/org/apache/storm/healthcheck/HealthChecker.java ---
@@ -106,13 +106,13 @@ public void run() {
BufferedReader reader = new BufferedReader(new InputStreamReader(stdin));
while ((str = reader.readLine()) != null) {
if (str.startsWith("ERROR")) {
+ LOG.warn("The healthcheck process {} exited with code {}", script, process.exitValue());
return FAILED;
}
}
return SUCCESS;
--- End diff --
Should this not failed with `return FAILED_WITH_EXIT_CODE;` since the exit code is not 0 ?
---
[GitHub] storm issue #2855: STORM-3240 health checks should succeed on exit code 0
Posted by agresch <gi...@git.apache.org>.
Github user agresch commented on the issue:
https://github.com/apache/storm/pull/2855
@revans2 - I am not familiar with previous releases. Can you explain what you mean by something changing in this release? Are you saying this bug also exists in the other lines?
---
[GitHub] storm issue #2855: STORM-3240 health checks should succeed on exit code 0
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2855
And if we can get rid of the code returning a string for success or failure, please lets do that, especially since it is thrown away.
---
[GitHub] storm issue #2855: STORM-3240 health checks should succeed on exit code 0
Posted by agresch <gi...@git.apache.org>.
Github user agresch commented on the issue:
https://github.com/apache/storm/pull/2855
Missed @revans2 last comment. But the String is used here: https://github.com/apache/storm/blob/master/storm-server/src/main/java/org/apache/storm/healthcheck/HealthChecker.java#L60
---
[GitHub] storm pull request #2855: STORM-3240 health checks should succeed on exit co...
Posted by agresch <gi...@git.apache.org>.
Github user agresch commented on a diff in the pull request:
https://github.com/apache/storm/pull/2855#discussion_r222100581
--- Diff: storm-server/src/main/java/org/apache/storm/healthcheck/HealthChecker.java ---
@@ -106,13 +106,13 @@ public void run() {
BufferedReader reader = new BufferedReader(new InputStreamReader(stdin));
while ((str = reader.readLine()) != null) {
if (str.startsWith("ERROR")) {
+ LOG.warn("The healthcheck process {} exited with code {}", script, process.exitValue());
return FAILED;
}
}
return SUCCESS;
--- End diff --
I can code this if desired, but the documentation had indicated that an ERROR was necessary to fail. I'm fine adding this change if people agree. Just let me know.
---