Skip to content

ZEPPELIN-46: set only non-empty values for for spark.* properties#38

Closed
bzz wants to merge 11 commits intoapache:masterfrom
bzz:fix-non-empty-spark-conf-paraments
Closed

ZEPPELIN-46: set only non-empty values for for spark.* properties#38
bzz wants to merge 11 commits intoapache:masterfrom
bzz:fix-non-empty-spark-conf-paraments

Conversation

@bzz
Copy link
Member

@bzz bzz commented Apr 16, 2015

@bzz
Copy link
Member Author

bzz commented Apr 16, 2015

So far this does not have any tests.
Thoughts on how\where to test this are warmly welcome!

Copy link
Member

Choose a reason for hiding this comment

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

@bzz Don't you mean !key.startsWith("spark.")?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jongyoul indeed! Thank you.
How can we test this except extracting a new helper method and testing it separately?

@jongyoul
Copy link
Member

@bzz Simply add this test in SparkInterpreterTest.java

@Test
public void testSparkConfCorrect() {
  for (Tuple2<String, String> tuple2 : repl.getSparkContext().getConf().getAll()) {
    if (tuple2._1().startsWith("spark."))
      assertFalse(String.format("configuration starting from 'spark.' should not be empty. [%s]: [%s]", tuple2._1(), tuple2._2()), tuple2._2().isEmpty());
  }
}

@jongyoul
Copy link
Member

@bzz My test is not perfect. I'll change it

@jongyoul
Copy link
Member

screen shot 2015-04-16 at 9 09 29 pm

@bzz I've found strange thing while I'm changing my test cases, why do you know intpProperty's size is 0 if I'm running debugging?

And this is a live screenshot for sparkConf. It works fine but I don't know why something's wrong.
screen shot 2015-04-16 at 9 22 54 pm

EDIT:
I've found the reason and make a PR #41

Copy link
Member

Choose a reason for hiding this comment

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

what about if val is whitespace? eg. " "

Copy link
Member

Choose a reason for hiding this comment

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

@felixcheung trim() will make whitespace removed. Tests case is not covered all of cases. I make a PR of @bzz and he will fix this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then val.trim().isEmpty() is true, so it will be set for any property, that does not start with "spark." (i.e whitespace as a password)

Copy link
Member

Choose a reason for hiding this comment

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

@bzz Sorry, I didn't reformat this. This needs one space between 'if' and '('.

@bzz
Copy link
Member Author

bzz commented Apr 17, 2015

Ready to merge

@felixcheung
Copy link
Member

LGTM

@syepes
Copy link
Contributor

syepes commented Apr 17, 2015

I have just applied this patch and there still seems to be an issue.
If I change the property "spark.executor.memory = 20g" and restart Zeppelin with debugging, It still uses the default value of "512m" knowing that in the interpreter.json file its set to 20g

@Leemoonsoo
Copy link
Member

Tested and LGTM.

for the problem @syepes mentioned, I have created fix #42

@bzz
Copy link
Member Author

bzz commented Apr 20, 2015

Merging if there is no discussion

@asfgit asfgit closed this in cdd343b Apr 20, 2015
@bzz bzz deleted the fix-non-empty-spark-conf-paraments branch June 11, 2015 09:55
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