Skip to content

[SPARK-11631][Scheduler] Adding 'Starting DAGScheduler' log #9603

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

[SPARK-11631][Scheduler] Adding 'Starting DAGScheduler' log #9603

wants to merge 2 commits into from

Conversation

xguo27
Copy link
Contributor

@xguo27 xguo27 commented Nov 10, 2015

No description provided.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@@ -506,6 +506,7 @@ class SparkContext(config: SparkConf) extends Logging with ExecutorAllocationCli
val (sched, ts) = SparkContext.createTaskScheduler(this, master)
_schedulerBackend = sched
_taskScheduler = ts
logDebug("Starting DAGScheduler")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather move it to DAGScheduler's constructor as that's where the message belongs.

@andrewor14
Copy link
Contributor

Meh, I don't really think this is worth doing. It doesn't add any value or fix anything.

@xguo27
Copy link
Contributor Author

xguo27 commented Nov 17, 2015

I agree it is trivial, just thought I could quickly add a log statement. If Jacek agrees, I can close those PR.

@jaceklaskowski
Copy link
Contributor

I don't agree with @andrewor14. It does add a value of being consistent with how Spark informs about its status - if it says "Stopping..." at INFO it should be corresponding "Starting..." at INFO. That was my initial goal. Consistency is the goal and the value here.

Why does Spark report about services being stopped at all? What's the rationale?

I would change it to DEBUG, but since all the other logs about starting services are at INFO, I'd leave it as is until someone reports it should be done.

@xguo27
Copy link
Contributor Author

xguo27 commented Nov 23, 2015

@andrewor14 What is your take on Jacek's comment? I don't think it's a bad idea to make it more consistent with a matching log message. Please let me know. Thx!

@andrewor14
Copy link
Contributor

I just don't see any value. It's obvious that the scheduler is starting / stopping. Not a big deal if we merge it, but I just can't imagine anyone finding this message useful.

@markhamstra
Copy link
Contributor

Beyond that, the message is actually somewhat misleading. The "Stopping" message occurs in stop(), which is responsible for stopping the messageScheduler, eventProcessLoop and taskScheduler. When the "Starting" message is logged, at least the eventProcessLoop and taskScheduler will already have been started, so the argument from symmetry for "Starting" because there is a "Stopping" doesn't hold even without considering that "Starting", ...logMessage, ...logMessage, ... isn't symmetrical to logMessage, ...logMessage, "Stopping" unless we can somehow run time both forwards and backwards. There is also the problem that the "Starting" message is only sent from the secondary constructors and not from the primary.

@rxin
Copy link
Contributor

rxin commented Nov 24, 2015

I think this message is complete useless. I'd just remove it rather than spending time bickering about the wording.

@xguo27
Copy link
Contributor Author

xguo27 commented Nov 24, 2015

OK, I will close it. Thanks!

@xguo27 xguo27 closed this Nov 24, 2015
@xguo27 xguo27 deleted the SPARK-11631 branch November 24, 2015 08:12
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.

6 participants