Skip to content

Conversation

@WangTaoTheTonic
Copy link
Contributor

@SparkQA
Copy link

SparkQA commented Sep 19, 2014

QA tests have started for PR 2454 at commit 2a79f26.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 19, 2014

QA tests have finished for PR 2454 at commit 2a79f26.

  • This patch passes unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • sealed trait Matrix extends Serializable
    • class SparseMatrix(
    • sealed trait Vector extends Serializable

@vanzin
Copy link
Contributor

vanzin commented Sep 20, 2014

LGTM. Could you fix the typo in the PR title (loaing)?

@WangTaoTheTonic WangTaoTheTonic changed the title [SPARK-3599]Avoid loaing properties file frequently [SPARK-3599]Avoid loading properties file frequently Sep 20, 2014
@WangTaoTheTonic
Copy link
Contributor Author

@vanzin Sorry for that. Fixed.

@SparkQA
Copy link

SparkQA commented Sep 20, 2014

QA tests have started for PR 2454 at commit 7dca036.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 20, 2014

QA tests have started for PR 2454 at commit 3681182.

  • This patch merges cleanly.

@WangTaoTheTonic
Copy link
Contributor Author

I think it is better using lazy val for readability(putting all elements of defaultSparkProperties into value properties is more comfortable than conversely) and not using clone to avoid shallow copy( I did not do deep research about clone methods in scala and take it same as in Java. Please point it out if not).

@SparkQA
Copy link

SparkQA commented Sep 20, 2014

QA tests have finished for PR 2454 at commit 7dca036.

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

@SparkQA
Copy link

SparkQA commented Sep 20, 2014

QA tests have finished for PR 2454 at commit 3681182.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

it's canonical to not use new here.

i.e.

val properties = HashMap[String, String]()

@pwendell
Copy link
Contributor

I don't think the overhead of reading the file is significant here, but agree it's nice to avoid extra print statements. lazy val's like this are a little brittle because they make the access order sensitive. I.e. if someone tries to reference this earlier in the program it will break in a weird way.

Nonetheless think this is fine to have. It's a small enough code segment here. I'll merge it and make the minor style fix suggested here.

@asfgit asfgit closed this in 8e875d2 Sep 21, 2014
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