[SPARK-5964] Allow spark-daemon.sh to support foreground operation #3881

hellertime · 2015-01-02T21:41:27Z

Add --foreground option to spark-daemon.sh to prevent the process from daemonizing itself. Useful if running under a watchdog which waits on its child process.

andrewor14 · 2015-02-19T21:40:13Z

@hellertime would you mind filing a JIRA for this PR?

JoshRosen · 2015-02-23T23:45:37Z

Hi @hellertime,

A really good use-case for this PR has just cropped up at #4720; I think that the --foreground flag will make it easier to write tests for certain components, such as the Hive ThriftServer. Do you think that you'll have time to work on this PR? If not, we can take it over.

JoshRosen · 2015-02-23T23:46:23Z

@nchammas, it might be good to get your eyes on this one given your shell scripting expertise.

liancheng · 2015-02-24T02:20:20Z

Noticed that indentation of spark-daemon.sh is a bit messy. But in general, we use 2-space indentation in shell scripts. Shall we at least use 2-space indentation for newly added code?

JoshRosen · 2015-02-24T02:35:46Z

sbin/spark-daemon.sh

+            echo "failed to launch $command:"
+            tail -2 "$log" | sed 's/^/  /'
+            echo "full log in $log"
+        fi
    else


Minor coding style thing, but could we add explicit comments to the else branches to help reiterate what case it corresponds to? With the deep nesting of else statements here, it's a little easy to get lost and not spot that this one refers to the "run in background / daemonize" case. So, maybe just write something like

else # run in background

would help.

hellertime · 2015-02-24T03:52:05Z

Ok. JIRA ticket has been filed, noted ticket in the title of this PR.

Happy to add the additional comment.

nchammas · 2015-02-24T04:12:34Z

sbin/spark-daemon.sh

-      echo "full log in $log"
+    if [ "$RUN_IN_FOREGROUND" = "0" ]; then
+      spark_rotate_log "$log"
+      echo starting $command, logging to $log


Is there a reason this was unquoted? I think it should be

echo "starting $command, logging to $log"

otherwise there is room for word splitting bugs.

nchammas · 2015-02-24T04:23:29Z

Sorry to spam this PR. Most of the word splitting issues are in code this PR didn't introduce, but since we're touching it here it's good to fix it up. Hope it's not too much @hellertime!

JoshRosen · 2015-02-24T04:44:06Z

Wow, very thorough review :)

Let me whitelist this PR with Jenkins so that it gets tested when you push changes.

JoshRosen · 2015-02-24T04:44:09Z

Jenkins, this is ok to test.

nchammas · 2015-02-24T05:19:19Z

On a separate, less important note, is there an easy way to add the new command line parameter in a position-independent way?

Right now it looks like --foreground can only be specified as the first option; dunno if there's a better approach.

SparkQA · 2015-02-24T06:05:35Z

Test build #27881 has finished for PR 3881 at commit 0995ee1.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

hellertime · 2015-02-24T16:03:59Z

@nchammas wow, happy to make those edits. As for the long options parsing order, the approach currently used by the script (and the approach that I coopted) is limited. It would require some rewrite in order to allow for parsing the args out of order.

Changes like that might be better in another PR, but it wouldn't be trouble to add them here, just a bit overreaching.

nchammas · 2015-02-24T16:17:41Z

Changes like that might be better in another PR

Agreed.

**NOTICE** Do NOT merge this, as we're waiting for #3881 to be merged. `HiveThriftServer2Suite` has been notorious for its flakiness for a while. This was mostly due to spawning and communicate with external server processes. This PR revamps this test suite for better robustness: 1. Fixes a racing condition occurred while using `tail -f` to check log file It's possible that the line we are looking for has already been printed into the log file before we start the `tail -f` process. This PR uses `tail -n +0 -f` to ensure all lines are checked. 2. Retries up to 3 times if the server fails to start In most of the cases, the server fails to start because of port conflict. This PR no longer asks the system to choose an available TCP port, but uses a random port first, and retries up to 3 times if the server fails to start. 3. A server instance is reused among all test cases within a single suite The original `HiveThriftServer2Suite` is splitted into two test suites, `HiveThriftBinaryServerSuite` and `HiveThriftHttpServerSuite`. Each suite starts a `HiveThriftServer2` instance and reuses it for all of its test cases. **TODO** - [ ] Starts the Thrift server in foreground once #3881 is merged (adding `--foreground` flag to `spark-daemon.sh`)  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/apache/spark/4720)  Author: Cheng Lian <lian@databricks.com> Closes #4720 from liancheng/revamp-thrift-server-tests and squashes the following commits: d6c80eb [Cheng Lian] Relaxes server startup timeout 6f14eb1 [Cheng Lian] Revamped HiveThriftServer2Suite for robustness (cherry picked from commit f816e73) Signed-off-by: Cheng Lian <lian@databricks.com>

**NOTICE** Do NOT merge this, as we're waiting for #3881 to be merged. `HiveThriftServer2Suite` has been notorious for its flakiness for a while. This was mostly due to spawning and communicate with external server processes. This PR revamps this test suite for better robustness: 1. Fixes a racing condition occurred while using `tail -f` to check log file It's possible that the line we are looking for has already been printed into the log file before we start the `tail -f` process. This PR uses `tail -n +0 -f` to ensure all lines are checked. 2. Retries up to 3 times if the server fails to start In most of the cases, the server fails to start because of port conflict. This PR no longer asks the system to choose an available TCP port, but uses a random port first, and retries up to 3 times if the server fails to start. 3. A server instance is reused among all test cases within a single suite The original `HiveThriftServer2Suite` is splitted into two test suites, `HiveThriftBinaryServerSuite` and `HiveThriftHttpServerSuite`. Each suite starts a `HiveThriftServer2` instance and reuses it for all of its test cases. **TODO** - [ ] Starts the Thrift server in foreground once #3881 is merged (adding `--foreground` flag to `spark-daemon.sh`)  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/apache/spark/4720)  Author: Cheng Lian <lian@databricks.com> Closes #4720 from liancheng/revamp-thrift-server-tests and squashes the following commits: d6c80eb [Cheng Lian] Relaxes server startup timeout 6f14eb1 [Cheng Lian] Revamped HiveThriftServer2Suite for robustness

AmplabJenkins · 2015-04-27T18:22:54Z

Can one of the admins verify this patch?

andrewor14 · 2015-06-18T23:58:30Z

@hellertime have you had the chance to address those comments? This patch has mostly gone stale at this point and I would recommend that we close it for now. If you prefer feel free to open a new updated one.

hellertime · 2015-06-21T03:17:35Z

@andrewor14 I was under the impression all the shell scripts were getting refactored and that this patch had become obsolete. I agree it's best to close this out.

Sent from my iPhone

On Jun 18, 2015, at 7:59 PM, andrewor14 notifications@github.com wrote:

@hellertime have you had the chance to address those comments? This patch has mostly gone stale at this point and I would recommend that we close it for now. If you prefer feel free to open a new updated one.

—
Reply to this email directly or view it on GitHub.

srowen · 2015-06-21T10:45:29Z

@hellertime you will have to close it

andrewor14 · 2015-06-22T18:04:04Z

Yes, also most scripts are being refactored into python so there's less of a reason to do this. (Though the spark-daemon.sh ones are still in BASH). Thanks.

zasdfgbnm · 2016-07-02T14:39:07Z

Can anyone explain what happened to this PR? Why people close this PR without adding any support for running in foreground?

srowen · 2016-07-02T15:18:30Z

It's right there at #3881 (comment)

## What changes were proposed in this pull request? Add a SPARK_NO_DAEMONIZE environment variable flag to spark-daemon.sh that causes the process it would run to be run in the foreground. It looks like there has been some prior work in apache#3881, but there was some talk about these being refactored. I'm not sure if that happened or not, but that PR is almost 2 years old at this point so it was worth revisiting. ## How was this patch tested? ./dev/run-tests still seems to work. It doesn't look like these scripts have tests, but if I missed them just let me know. Author: Mike Ihbe <mikejihbe@gmail.com> Closes apache#15338 from mikejihbe/SPARK-11653.

JoshRosen mentioned this pull request Feb 23, 2015

[SPARK-5751] [SQL] Revamped HiveThriftServer2Suite for robustness #4720

Closed

1 task

JoshRosen reviewed Feb 24, 2015
View reviewed changes

hellertime changed the title ~~Allow spark-daemon.sh to support foreground operation~~ [SPARK-5964] Allow spark-daemon.sh to support foreground operation Feb 24, 2015

hellertime added 2 commits February 23, 2015 23:04

Allow spark-daemon.sh to support foreground operation

0647c6b

Adjust indentation on new code

0995ee1

nchammas reviewed Feb 24, 2015
View reviewed changes

hellertime closed this Jun 21, 2015

zasdfgbnm mentioned this pull request Jul 2, 2016

[SPARK-16352] Spark daemon run.sh #14027

Closed

mikejihbe mentioned this pull request Oct 4, 2016

[SPARK-11653][Deploy] Allow spark-daemon.sh to run in the foreground #15338

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-5964] Allow spark-daemon.sh to support foreground operation #3881

[SPARK-5964] Allow spark-daemon.sh to support foreground operation #3881

hellertime commented Jan 2, 2015

andrewor14 commented Feb 19, 2015

JoshRosen commented Feb 23, 2015

JoshRosen commented Feb 23, 2015

liancheng commented Feb 24, 2015

JoshRosen Feb 24, 2015

hellertime commented Feb 24, 2015

nchammas Feb 24, 2015

nchammas commented Feb 24, 2015

JoshRosen commented Feb 24, 2015

JoshRosen commented Feb 24, 2015

nchammas commented Feb 24, 2015

SparkQA commented Feb 24, 2015

hellertime commented Feb 24, 2015

nchammas commented Feb 24, 2015

AmplabJenkins commented Apr 27, 2015

andrewor14 commented Jun 18, 2015

hellertime commented Jun 21, 2015

srowen commented Jun 21, 2015

andrewor14 commented Jun 22, 2015

zasdfgbnm commented Jul 2, 2016

srowen commented Jul 2, 2016

[SPARK-5964] Allow spark-daemon.sh to support foreground operation #3881

[SPARK-5964] Allow spark-daemon.sh to support foreground operation #3881

Conversation

hellertime commented Jan 2, 2015

andrewor14 commented Feb 19, 2015

JoshRosen commented Feb 23, 2015

JoshRosen commented Feb 23, 2015

liancheng commented Feb 24, 2015

JoshRosen Feb 24, 2015

Choose a reason for hiding this comment

hellertime commented Feb 24, 2015

nchammas Feb 24, 2015

Choose a reason for hiding this comment

nchammas commented Feb 24, 2015

JoshRosen commented Feb 24, 2015

JoshRosen commented Feb 24, 2015

nchammas commented Feb 24, 2015

SparkQA commented Feb 24, 2015

hellertime commented Feb 24, 2015

nchammas commented Feb 24, 2015

AmplabJenkins commented Apr 27, 2015

andrewor14 commented Jun 18, 2015

hellertime commented Jun 21, 2015

srowen commented Jun 21, 2015

andrewor14 commented Jun 22, 2015

zasdfgbnm commented Jul 2, 2016

srowen commented Jul 2, 2016