Skip to content

Conversation

@ganeshkrishnan1
Copy link

@ganeshkrishnan1 ganeshkrishnan1 commented Oct 10, 2016

This is the Spark Scala example which was missing setting a master URL in Spark Session

Unit tested. Changes affect examples and documentation only

There is no UI change. It's a minor change in the example scala files.

Need to set master url to SparkSession for the example to run

Need to set master url to SparkSession for the example to run
@rxin
Copy link
Contributor

rxin commented Oct 10, 2016

LGTM - can you clean up the pr description to remove the messages from the template?

@hhbyyh
Copy link
Contributor

hhbyyh commented Oct 10, 2016

@rxin Shall we set default master for all the examples, or this is a special case?

@srowen
Copy link
Member

srowen commented Oct 12, 2016

Ping @getintouchapp

@ganeshkrishnan1
Copy link
Author

Am I supposed to do anything here? I cleaned up the comments and I don't have permission to do anything else beyond that.

@srowen
Copy link
Member

srowen commented Oct 12, 2016

Yes thanks for updating the description. The title could be better. Yuhao also asked if this is something that looks like it needs to be done for more examples?

@ganeshkrishnan1 ganeshkrishnan1 changed the title Updated master url Set master URL configuration in scala example Oct 12, 2016
@ganeshkrishnan1
Copy link
Author

Fixed the title. Yes other examples are missing the master url config too. I will add them and create a pull request for the rest of the files once this is merged.

@ganeshkrishnan1
Copy link
Author

btw, for the rest of the files its not a major issue because they don't turn up in documentation. This one does at http://spark.apache.org/docs/latest/sql-programming-guide.html#starting-point-sparksession

@srowen
Copy link
Member

srowen commented Oct 12, 2016

I think it's right to do this all at once, rather than in pieces.

I must say I recall I opened a PR like this a long long time ago and Matei said the master was excluded on purpose because it was intended to be set by the environment running the example, which made some sense.

I wonder if the logic is still the same or not? that is, if it's a runnable example, do we want to not override the master that the runner might set? or for doc-only example code, is it that we do need to show master being set programmatically?

@ganeshkrishnan1
Copy link
Author

It does make sense to load master url dynamically thru environment or command line while running the example. However the documentation example should be a valid piece of code and not dependent on other hidden variables.

I vote that we merge this pull request for the sake of clarity in documentation but for other examples we leave as is, since they could load the master URL dynamically

@srowen
Copy link
Member

srowen commented Oct 12, 2016

OK are there other such examples that are of the same form? it makes sense to change any case like this in one go.

@SparkQA
Copy link

SparkQA commented Oct 12, 2016

Test build #3333 has finished for PR 15411 at commit 5324034.

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

@srowen
Copy link
Member

srowen commented Oct 15, 2016

Hm, so I see several more examples that get included in documentation that don't set master. I am not sure that is a salient difference, because in general, when writing your own app you would not hard-code a master in the code either. The examples evidently don't generally set master for this reason. Therefore I'm not sure we shoudl make this change.

srowen added a commit to srowen/spark that referenced this pull request Oct 31, 2016
@asfgit asfgit closed this in 26b07f1 Oct 31, 2016
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.

5 participants