Skip to content
This repository was archived by the owner on Jan 9, 2020. It is now read-only.

Conversation

@erikerlandson
Copy link
Member

I ran into some exceptions being thrown from the endpoint watcher in the submission client. It turned out that null values were being returned for endpoints.getSubsets, instead of just empty collection. I added some guarding checks for null, which fixed the problem.

I also modified the parentheses around action types, because that appeared incorrect. I believe it should be checking for (action == added || action == modified) as a unit in the first clause.

@erikerlandson erikerlandson requested review from foxish and mccheah May 8, 2017 20:24
@erikerlandson
Copy link
Member Author

Anybody else getting build fail? It's failing in core, which this PR makes no changes to.

@erikerlandson
Copy link
Member Author

jenkins please test this

@foxish
Copy link
Member

foxish commented May 8, 2017

rerun unit tests please

@foxish
Copy link
Member

foxish commented May 8, 2017

@erikerlandson I think it's a flake. The rerun should fix it.

@erikerlandson
Copy link
Member Author

Do these edits look OK to everyone else?

@foxish
Copy link
Member

foxish commented May 9, 2017

Taking a look now. Sorry about the delay.

@foxish
Copy link
Member

foxish commented May 9, 2017

I think endpoints itself can't be null, but defensively, it seems like a good idea to check for it. Agree on the parens being incorrect before. LGTM

Copy link

@ash211 ash211 left a comment

Choose a reason for hiding this comment

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

LGTM let's merge when the build passes

extends Watcher[Service] {
override def eventReceived(action: Action, service: Service): Unit = {
if ((action == Action.ADDED) || (action == Action.MODIFIED)
if ((action == Action.ADDED || action == Action.MODIFIED)
Copy link

Choose a reason for hiding this comment

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

the parentheses change slightly modifies the evaluation order of these expressions, and the new ordering seems correct.

Java precedence for boolean expressions is ! then && then || so previously this was evaluating the && first and then the ||

@ash211 ash211 merged commit 26f747e into apache-spark-on-k8s:branch-2.1-kubernetes May 10, 2017
foxish pushed a commit that referenced this pull request Jul 24, 2017
ifilonenko pushed a commit to ifilonenko/spark that referenced this pull request Feb 26, 2019
…nc-kube

[NOSQUASH] Resync with Kubernetes
puneetloya pushed a commit to puneetloya/spark that referenced this pull request Mar 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants