[SPARK-10955][streaming] Add a warning if dynamic allocation for Streaming applications #8998

harishreedharan · 2015-10-06T19:01:25Z

Dynamic allocation can be painful for streaming apps and can lose data. Log a warning for streaming applications if dynamic allocation is enabled.

…lications. Dynamic allocation can be painful for streaming apps and can lose data. The one drawback though is that apps which run Streaming and non-streaming form the same context will also end up not being able to do dynamic allocation.

harishreedharan · 2015-10-06T19:01:39Z

/cc @vanzin @tdas

harishreedharan · 2015-10-06T19:08:21Z

Actually looking at this again, I think our only option is to log a message. It is possible that the SparkContext was already created and passed to us, in which case we can't do anything about dynamic allocation.

JoshRosen · 2015-10-06T19:27:30Z

/cc @andrewor14

andrewor14 · 2015-10-06T19:36:31Z

This looks good. I personally prefer to fail fast loudly since this is a correctness issue. Many people don't pay attention to warnings these days, especially in hosted environments.

SparkQA · 2015-10-06T20:00:04Z

Test build #43289 has finished for PR 8998 at commit a4a5212.

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

SparkQA · 2015-10-06T20:08:25Z

Test build #43290 has finished for PR 8998 at commit b3f9a95.

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

vanzin · 2015-10-06T20:16:10Z

Yeah, looks fine to me. Perhaps an internal config option so that the check can be disabled? e.g. if someone wants to actually try it out and see how it behaves without having to recompile Spark.

…ly sets it.

harishreedharan · 2015-10-06T20:51:47Z

Added config parameter to enable it if the user really wants to enable dynamic allocation

markgrover · 2015-10-06T21:26:57Z

Pointing out the obvious here, but we should document this new property so someone doesn't have to read the source code to figure out what's the name of the property to enable dynamic allocation in streaming.

Also, I personally prefer the name spark.streaming.dynamicAllocation.allowed over spark.streaming.dynamicAllocation.enabled because one will still have to enable dynamic allocation for the entire spark context separately. And, as much as I don't like the idea of adding yet another property I don't really see a better way to do this, essentially we want to decouple folks from enabling dynamic allocation for streaming vs. non-streaming workloads. Currently, we use the same property, we could decouple by having 2 separate properties - one for streaming and one for the rest, or, alternatively, as is being suggested here, have a second property to allow/disallow dynamic allocation for streaming. The latter seems less hairy so I am for it.

vanzin · 2015-10-06T21:28:43Z

but we should document this new property

If you document it, it becomes public, and you need to document that you removed it, and all sorts of things. Since the point is not to publicize it, instead just have a way for those tinkering with Spark itself to enable the feature, documentation is not necessary.

SparkQA · 2015-10-06T21:50:16Z

Test build #43299 has finished for PR 8998 at commit 725f090.

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

harishreedharan · 2015-10-06T21:55:27Z

I don't have a strong preference for the config name. @vanzin, @andrewor14 - like the current name or the one which @markgrover suggested? Vote please :)

tdas · 2015-10-06T22:08:29Z

I strongly think that we should not add that require. It can break existing streaming applications very easily. People might be using dynamic allocation with direct kafka or kinesis or with anything + WAL, in which case it is fine to have dynamic allocation as the system will not loose data. Upgrading Spark will immediately break their application, and it might be hard to figure out the solution for disabling dynamic allocation in their setting.

We have made mistakes in the past by throwing errors like this for usecases which we didnt think would be useful in production, and people have complained that it broke their testing stuff. See https://issues.apache.org/jira/browse/SPARK-8630 and discussion https://www.mail-archive.com/search?l=user@spark.apache.org&q=subject:%22Re%5C%3A+QueueStream+Does+Not+Support+Checkpointing%22&o=newest&f=1

We must not break stuff. So I suggest this be turned into a warning. In that case we dont need a new configuration.

tdas · 2015-10-06T22:09:16Z

Also please update the PR title to be similar to other PRs.

harishreedharan · 2015-10-06T22:15:10Z

As I said, I don't mind either. But in this case, we need to be conservative. We can additional checks to see if WAL is enabled, but it is not possible to actually check what DStreams are being used (it could be a custom one, which does not break if executors go away), so that case still exists.

How about we print the config param out or document it in that case?

harishreedharan · 2015-10-06T23:29:15Z

@tdas What do you think about the above? If you still think we should just make it a warn, I will make the change.

tdas · 2015-10-06T23:33:21Z

Checking which types of dstreams are used and what configurations are used and accordingly throwing errors seems to be complicated and brittle. I think its best to log a warning.

In general, if they are not using WAL or reliable sources, they should be aware that they are going to loose data under failures anyways, and they should be aware of that anyways. Dynamic allocation really does not aggravate the problem significantly more.

…n is enabled.

tdas · 2015-10-06T23:52:23Z

streaming/src/main/scala/org/apache/spark/streaming/StreamingContext.scala

+    if (Utils.isDynamicAllocationEnabled(sc.conf)) {
+      logWarning("Dynamic Allocation is enabled for this application. " +
+        "Enabling Dynamic allocation for Spark Streaming applications can cause data loss if " +
+        "Write Ahead Log is not enabled for non-replayable sources, like Flume.")


remove ","
You can also add "See the documentation for enabling the Write Ahead Log".

SparkQA · 2015-10-07T00:41:13Z

Test build #43306 has finished for PR 8998 at commit eaa48cc.

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

SparkQA · 2015-10-07T01:36:30Z

Test build #43308 has finished for PR 8998 at commit 2733d94.

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

tdas · 2015-10-07T02:13:49Z

@harishreedharan you missed one small point, probably because i edited the comment later. Could you also add "See the programming guide for enabling the Write Ahead Log" .

tdas · 2015-10-07T02:13:57Z

Other than that its LGTM.

SparkQA · 2015-10-07T18:53:25Z

Test build #43330 has finished for PR 8998 at commit 462b264.

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

tdas · 2015-10-09T01:52:37Z

LGTM, merging this to master and branch 1.5

…eaming applications Dynamic allocation can be painful for streaming apps and can lose data. Log a warning for streaming applications if dynamic allocation is enabled. Author: Hari Shreedharan <hshreedharan@apache.org> Closes #8998 from harishreedharan/ss-log-error and squashes the following commits: 462b264 [Hari Shreedharan] Improve log message. 2733d94 [Hari Shreedharan] Minor change to warning message. eaa48cc [Hari Shreedharan] Log a warning instead of failing the application if dynamic allocation is enabled. 725f090 [Hari Shreedharan] Add config parameter to allow dynamic allocation if the user explicitly sets it. b3f9a95 [Hari Shreedharan] Disable dynamic allocation and kill app if it is enabled. a4a5212 [Hari Shreedharan] [streaming] SPARK-10955. Disable dynamic allocation for Streaming applications. (cherry picked from commit 0984129) Signed-off-by: Tathagata Das <tathagata.das1565@gmail.com>

…eaming applications Dynamic allocation can be painful for streaming apps and can lose data. Log a warning for streaming applications if dynamic allocation is enabled. Author: Hari Shreedharan <hshreedharan@apache.org> Closes apache#8998 from harishreedharan/ss-log-error and squashes the following commits: 462b264 [Hari Shreedharan] Improve log message. 2733d94 [Hari Shreedharan] Minor change to warning message. eaa48cc [Hari Shreedharan] Log a warning instead of failing the application if dynamic allocation is enabled. 725f090 [Hari Shreedharan] Add config parameter to allow dynamic allocation if the user explicitly sets it. b3f9a95 [Hari Shreedharan] Disable dynamic allocation and kill app if it is enabled. a4a5212 [Hari Shreedharan] [streaming] SPARK-10955. Disable dynamic allocation for Streaming applications.

Disable dynamic allocation and kill app if it is enabled.

b3f9a95

Add config parameter to allow dynamic allocation if the user explicit…

725f090

…ly sets it.

harishreedharan force-pushed the ss-log-error branch from 463fd1a to 725f090 Compare October 6, 2015 20:50

harishreedharan changed the title ~~[streaming] SPARK-10955. Disable dynamic allocation for Streaming app…~~ [SPARK-10955][streaming] Disable dynamic allocation for Streaming app… Oct 6, 2015

Log a warning instead of failing the application if dynamic allocatio…

eaa48cc

…n is enabled.

harishreedharan force-pushed the ss-log-error branch from 5ea6690 to eaa48cc Compare October 6, 2015 23:41

tdas reviewed Oct 6, 2015
View reviewed changes

Minor change to warning message.

2733d94

Improve log message.

462b264

harishreedharan changed the title ~~[SPARK-10955][streaming] Disable dynamic allocation for Streaming app…~~ [SPARK-10955][streaming] Add a warning if dynamic allocation for Streaming applications Oct 7, 2015

asfgit closed this in 0984129 Oct 9, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-10955][streaming] Add a warning if dynamic allocation for Streaming applications #8998

[SPARK-10955][streaming] Add a warning if dynamic allocation for Streaming applications #8998

harishreedharan commented Oct 6, 2015

harishreedharan commented Oct 6, 2015

harishreedharan commented Oct 6, 2015

JoshRosen commented Oct 6, 2015

andrewor14 commented Oct 6, 2015

SparkQA commented Oct 6, 2015

SparkQA commented Oct 6, 2015

vanzin commented Oct 6, 2015

harishreedharan commented Oct 6, 2015

markgrover commented Oct 6, 2015

vanzin commented Oct 6, 2015

SparkQA commented Oct 6, 2015

harishreedharan commented Oct 6, 2015

tdas commented Oct 6, 2015

tdas commented Oct 6, 2015

harishreedharan commented Oct 6, 2015

harishreedharan commented Oct 6, 2015

tdas commented Oct 6, 2015

tdas Oct 6, 2015

SparkQA commented Oct 7, 2015

SparkQA commented Oct 7, 2015

tdas commented Oct 7, 2015

tdas commented Oct 7, 2015

SparkQA commented Oct 7, 2015

tdas commented Oct 9, 2015

[SPARK-10955][streaming] Add a warning if dynamic allocation for Streaming applications #8998

[SPARK-10955][streaming] Add a warning if dynamic allocation for Streaming applications #8998

Conversation

harishreedharan commented Oct 6, 2015

harishreedharan commented Oct 6, 2015

harishreedharan commented Oct 6, 2015

JoshRosen commented Oct 6, 2015

andrewor14 commented Oct 6, 2015

SparkQA commented Oct 6, 2015

SparkQA commented Oct 6, 2015

vanzin commented Oct 6, 2015

harishreedharan commented Oct 6, 2015

markgrover commented Oct 6, 2015

vanzin commented Oct 6, 2015

SparkQA commented Oct 6, 2015

harishreedharan commented Oct 6, 2015

tdas commented Oct 6, 2015

tdas commented Oct 6, 2015

harishreedharan commented Oct 6, 2015

harishreedharan commented Oct 6, 2015

tdas commented Oct 6, 2015

tdas Oct 6, 2015

Choose a reason for hiding this comment

SparkQA commented Oct 7, 2015

SparkQA commented Oct 7, 2015

tdas commented Oct 7, 2015

tdas commented Oct 7, 2015

SparkQA commented Oct 7, 2015

tdas commented Oct 9, 2015