Skip to content

Conversation

@rxin
Copy link
Contributor

@rxin rxin commented Feb 25, 2016

What changes were proposed in this pull request?

This patch creates the public API for runtime configuration and an implementation for it. The public runtime configuration includes configs for existing SQL, as well as Hadoop Configuration.

This new interface is currently dead code. It will be added to SQLContext and a session entry point to Spark when we add that.

How was this patch tested?

a new unit test suite

Remove SparkSession

unit test

fix compilation

more tests and methods
@rxin
Copy link
Contributor Author

rxin commented Feb 25, 2016

cc @marmbrus

@SparkQA
Copy link

SparkQA commented Feb 26, 2016

Test build #52003 has finished for PR 11378 at commit 74fac3f.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • abstract class RuntimeConfig
    • class RuntimeConfigImpl extends RuntimeConfig

@yhuai
Copy link
Contributor

yhuai commented Feb 26, 2016

LGTM. Merging to master.

@asfgit asfgit closed this in 26ac608 Feb 26, 2016
*
* @since 2.0.0
*/
def set(key: String, value: Long): RuntimeConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have getBoolean and getLong, which means we don't really store config values as Boolean or Long. So why not we implement these 2 set in the base class, i.e. turn Boolean or Long to string and call the abstract set?The get returns String anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any possibilities that we will implement the boolean and long set specially? i.e. not just turn it to string and set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it is a lot more annoying to have to type

.set("key", "1.0")

than just

.set("key", 1.0)

On Thursday, February 25, 2016, Wenchen Fan notifications@github.com
wrote:

In sql/core/src/main/scala/org/apache/spark/sql/RuntimeConfig.scala
#11378 (comment):

  • */
  • def set(key: String, value: String): RuntimeConfig
  • /**
  • * Sets the given Spark runtime configuration property.
  • * @SInCE 2.0.0
  • */
  • def set(key: String, value: Boolean): RuntimeConfig
  • /**
  • * Sets the given Spark runtime configuration property.
  • * @SInCE 2.0.0
  • */
  • def set(key: String, value: Long): RuntimeConfig

We don't have getBoolean and getLong, which means we don't really store
config values as Boolean or Long. So why not we implement these 2 set in
the base class, i.e. turn Boolean or Long to string and call the abstract
set?The get returns String anyway.


Reply to this email directly or view it on GitHub
https://github.com/apache/spark/pull/11378/files#r54211858.

Copy link
Contributor

Choose a reason for hiding this comment

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

yea, I understand this 2 sets are useful, but I don't understand why we mark them abstract. It's just a toString and call the basic set, we can implement it in this base class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah ok.

it is just a lot cleaner here for this file to not have any implementation

asfgit pushed a commit that referenced this pull request Apr 26, 2016
## What changes were proposed in this pull request?

`RuntimeConfig` is the new user-facing API in 2.0 added in #11378. Until now, however, it's been dead code. This patch uses `RuntimeConfig` in `SessionState` and exposes that through the `SparkSession`.

## How was this patch tested?

New test in `SQLContextSuite`.

Author: Andrew Or <andrew@databricks.com>

Closes #12669 from andrewor14/use-runtime-conf.
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.

4 participants