Skip to content

Conversation

@tejasapatil
Copy link
Contributor

What changes were proposed in this pull request?

Currently the update interval for the console progress bar is hardcoded. This PR makes it configurable for users.

How was this patch tested?

Ran a long running job and with a high value of update interval, the updates were shown less frequently.

@srowen
Copy link
Member

srowen commented Aug 5, 2016

Is this worth making another knob to configure? Just disable it if it's at all annoying?

@tejasapatil
Copy link
Contributor Author

@srowen : Our daily jobs run remotely and users see the stdout / stderr logs in case they want to dig whats going on. With current update frequency, longer job's log tend to be flooded with console progress bar updates. Turning it off was an option but then one would have to click on the UI and correlate the timestamps with ones from the log (in case there is need to debug something). Setting the frequency to a higher value would give best of both the worlds.

@srowen
Copy link
Member

srowen commented Aug 5, 2016

Hm, would a higher value make sense? 200ms is pretty low. 3 seconds?
I get it, it seems like it's easy to just add a lever to pull to solve this, but it seems like this is a poor way to timestamp things anyway, and you really just want to disable the console progress bar, and perhaps look at better logging progress some other way.

@SparkQA
Copy link

SparkQA commented Aug 5, 2016

Test build #63271 has finished for PR 14507 at commit 596836a.

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

@tejasapatil
Copy link
Contributor Author

For batch jobs running for say ~10 hours, with 3 sec frequency, there would be 18k lines from the progress bar. That sounds like a lot. In Hadoop land they used to have 3 sec but it was made configurable [0] due to the same reason. See MAPREDUCE-6242 [1]. For ad hoc use cases, people would want to have faster response (eg. Presto updates every half a sec [2]). Coming up with a static value which works for both the worlds is hard.

To be precise, we don't rely on it for debugging issues. Its more for users so that they see that things are progressing and also while eye-balling to see if there is something obvious which went wrong.

[0] : https://github.com/apache/hadoop/blob/2e1d0ff4e901b8313c8d71869735b94ed8bc40a0/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapred/Task.java#L784
[1] : https://issues.apache.org/jira/browse/MAPREDUCE-6242
[2] : https://github.com/prestodb/presto/blob/master/presto-cli/src/main/java/com/facebook/presto/cli/StatusPrinter.java#L96

val CR = '\r'
// Update period of progress bar, in milliseconds
val UPDATE_PERIOD = 200L
val UPDATE_PERIOD_MSEC = if (SparkEnv.get == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be null, and, can't you use the context's conf here instead?
These can be private perhaps?
These constants really should have been in an object, but now that this sorta needs to be a field anyway, consider making its name more like a field name "updatePeriodMSec"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did suggested changes

@tejasapatil
Copy link
Contributor Author

Hive has hive.querylog.plan.progress.interval for the same purpose: https://cwiki.apache.org/confluence/display/Hive/AdminManual+Configuration . Given that its mostly used for batch, the interval is set to 1 min by default.

@srowen
Copy link
Member

srowen commented Aug 5, 2016

OK, seems reasonable to me.

@SparkQA
Copy link

SparkQA commented Aug 5, 2016

Test build #63278 has finished for PR 14507 at commit ab7e1e2.

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

@srowen
Copy link
Member

srowen commented Aug 8, 2016

Merged to master

@asfgit asfgit closed this in e076fb0 Aug 8, 2016
@tejasapatil tejasapatil deleted the SPARK-16919 branch August 9, 2016 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants