Skip to content

[SPARK-20519][SQL][CORE]Modify to prevent some possible runtime exceptions #17796

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[SPARK-20519][SQL][CORE]Modify to prevent some possible runtime exceptions #17796

wants to merge 1 commit into from

Conversation

@10110346
Copy link
Contributor

@10110346 10110346 commented Apr 28, 2017 •
edited
Loading

Signed-off-by: liuxian [email protected]

What changes were proposed in this pull request?

When the input parameter is null, may be a runtime exception occurs

How was this patch tested?

Existing unit tests

srowen
srowen reviewed Apr 28, 2017
View reviewed changes
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

Don't make JIRAs for these; these are borderline not worth proposing, please.

core/src/main/scala/org/apache/spark/util/Utils.scala Outdated
Copy link
Member

@srowen srowen Apr 28, 2017

Choose a reason for hiding this comment

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

This should be OK, but, it's already being checked for null in the line above.

@10110346 10110346 force-pushed the wip_lx_0428 branch 2 times, most recently from c5896c7 to e0eb912 Compare April 28, 2017 08:51
@10110346
Copy link
Contributor Author

10110346 commented Apr 28, 2017

ok, thanks for review it@srowen

srowen
srowen reviewed Apr 30, 2017
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/SQLContext.scala Outdated
Copy link
Member

@srowen srowen Apr 30, 2017

Choose a reason for hiding this comment

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

CC maybe @yhuai - OK to let tables(null) act like tables()? seems reasonable

@srowen
Copy link
Member

srowen commented May 4, 2017

@10110346 to be conservative, maybe remove the change to SQLContext. It's not clear to me that you are supposed to be able to call {{tables(null)}} because in this case you should call {{tables()}}. A NullPointerException seems like reasonable behavior.

However I think you could also expand the improvement for Utils.checkHost and checkHostPort. They all take this "message" argument which is pretty pointless. The message is a little different in many places and doesn't include the actual value most of the time. Just setting a complete valid message once in the method rather than taking it as an argument would further help.

@10110346
Copy link
Contributor Author

10110346 commented May 4, 2017

I quite agree with you.
I will do it,thanks @srowen

@SparkQA
Copy link

SparkQA commented May 4, 2017

Test build #3689 has finished for PR 17796 at commit 88b9e8c.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gatorsmile
Copy link
Member

gatorsmile commented May 5, 2017

ok to test

@SparkQA
Copy link

SparkQA commented May 5, 2017

Test build #76482 has started for PR 17796 at commit 0b7aac2.

@10110346
Copy link
Contributor Author

10110346 commented May 5, 2017 •
edited
Loading

Test FAILed. I don't know why.
Environment problem? Could you help me? thanks @gatorsmile

srowen
srowen requested changes May 5, 2017
View reviewed changes
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

Other than the message change, yes, looks good

core/src/main/scala/org/apache/spark/util/Utils.scala Outdated
Copy link
Member

@srowen srowen May 5, 2017

Choose a reason for hiding this comment

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

The message should be s"Expected hostname (not IP) but got $host"

core/src/main/scala/org/apache/spark/util/Utils.scala Outdated
Copy link
Member

@srowen srowen May 5, 2017

Choose a reason for hiding this comment

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

Likewise here

@SparkQA
Copy link

SparkQA commented May 6, 2017

Test build #76507 has finished for PR 17796 at commit 494928d.

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

@10110346
Copy link
Contributor Author

10110346 commented May 6, 2017

@srowen done, thank you very much.

srowen
srowen reviewed May 6, 2017
View reviewed changes
core/src/main/scala/org/apache/spark/util/Utils.scala Outdated
Copy link
Member

@srowen srowen May 6, 2017

Choose a reason for hiding this comment

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

Change "hostport" to "host and port"

@SparkQA
Copy link

SparkQA commented May 8, 2017

Test build #76557 has finished for PR 17796 at commit 0e879c8.

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

@srowen
Copy link
Member

srowen commented May 8, 2017

Merged to master

@asfgit asfgit closed this in 0f820e2 May 8, 2017
liyichao pushed a commit to liyichao/spark that referenced this pull request May 24, 2017
@10110346 @liyichao
[SPARK-20519][SQL][CORE] Modify to prevent some possible runtime exce…
17fd9b1 
…ptions

Signed-off-by: liuxian <liu.xian3zte.com.cn>

## What changes were proposed in this pull request?

When the input parameter is null, may be a runtime exception occurs

## How was this patch tested?
Existing unit tests

Author: liuxian <[email protected]>

Closes apache#17796 from 10110346/wip_lx_0428.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@srowen srowen srowen requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@10110346 @srowen @SparkQA @gatorsmile