Skip to content

Conversation

@yimin-yang
Copy link
Contributor

@yimin-yang yimin-yang commented Mar 17, 2022

What changes were proposed in this pull request?

Added null check for exec.metricValues.

Why are the changes needed?

When requesting Restful API {baseURL}/api/v1/applications/$appId/sql/$executionId which is introduced by this PR #28208, it can cause NullPointerException. The root cause is, when calling method doUpdate() of LiveExecutionData, metricsValues can be null. Then, when statement printableMetrics(graph.allNodes, exec.metricValues) is executed, it will throw NullPointerException.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Tested manually.

@github-actions github-actions bot added the SQL label Mar 17, 2022
@yimin-yang yimin-yang changed the title [SPARK-38579][WEBUI]Requesting Restful API can cause NullPointerException [SPARK-38579][SQL][WEBUI]Requesting Restful API can cause NullPointerException Mar 17, 2022
@yimin-yang yimin-yang force-pushed the fix-npe branch 3 times, most recently from 91f64b8 to 5e444bf Compare March 17, 2022 03:11
@yimin-yang yimin-yang force-pushed the fix-npe branch 2 times, most recently from 8d04d75 to 76f2a2e Compare March 17, 2022 05:37
@AmplabJenkins
Copy link

Can one of the admins verify this patch?

Copy link
Contributor

@weixiuli weixiuli left a comment

Choose a reason for hiding this comment

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

LGTM

printableMetrics(graph.allNodes, exec.metricValues)
} else {
printableMetrics(graph.allNodes, new HashMap[Long, String])
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Option(exec.metricValues).getOrElse(Map()) instead

@yimin-yang
Copy link
Contributor Author

@mridulm @weixiuli Please review this again, thanks!

@yimin-yang
Copy link
Contributor Author

@martin-g Could you merge this PR? Thx

@martin-g
Copy link
Member

I am just a contributor here.
Someone from the Spark team will merge it soon!

@wangyum wangyum closed this in 99992a4 Mar 22, 2022
wangyum pushed a commit that referenced this pull request Mar 22, 2022
…rException

### What changes were proposed in this pull request?

Added null check for `exec.metricValues`.

### Why are the changes needed?

When requesting Restful API  {baseURL}/api/v1/applications/$appId/sql/$executionId which is introduced by this PR #28208, it can cause NullPointerException. The root cause is, when calling method doUpdate() of `LiveExecutionData`, `metricsValues` can be null. Then, when statement `printableMetrics(graph.allNodes, exec.metricValues)` is executed, it will throw NullPointerException.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Tested manually.

Closes #35884 from yym1995/fix-npe.

Lead-authored-by: Yimin <[email protected]>
Co-authored-by: Yimin Yang <[email protected]>
Signed-off-by: Yuming Wang <[email protected]>
(cherry picked from commit 99992a4)
Signed-off-by: Yuming Wang <[email protected]>
wangyum pushed a commit that referenced this pull request Mar 22, 2022
…rException

### What changes were proposed in this pull request?

Added null check for `exec.metricValues`.

### Why are the changes needed?

When requesting Restful API  {baseURL}/api/v1/applications/$appId/sql/$executionId which is introduced by this PR #28208, it can cause NullPointerException. The root cause is, when calling method doUpdate() of `LiveExecutionData`, `metricsValues` can be null. Then, when statement `printableMetrics(graph.allNodes, exec.metricValues)` is executed, it will throw NullPointerException.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Tested manually.

Closes #35884 from yym1995/fix-npe.

Lead-authored-by: Yimin <[email protected]>
Co-authored-by: Yimin Yang <[email protected]>
Signed-off-by: Yuming Wang <[email protected]>
(cherry picked from commit 99992a4)
Signed-off-by: Yuming Wang <[email protected]>
wangyum pushed a commit that referenced this pull request Mar 22, 2022
…rException

### What changes were proposed in this pull request?

Added null check for `exec.metricValues`.

### Why are the changes needed?

When requesting Restful API  {baseURL}/api/v1/applications/$appId/sql/$executionId which is introduced by this PR #28208, it can cause NullPointerException. The root cause is, when calling method doUpdate() of `LiveExecutionData`, `metricsValues` can be null. Then, when statement `printableMetrics(graph.allNodes, exec.metricValues)` is executed, it will throw NullPointerException.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Tested manually.

Closes #35884 from yym1995/fix-npe.

Lead-authored-by: Yimin <[email protected]>
Co-authored-by: Yimin Yang <[email protected]>
Signed-off-by: Yuming Wang <[email protected]>
(cherry picked from commit 99992a4)
Signed-off-by: Yuming Wang <[email protected]>
@wangyum
Copy link
Member

wangyum commented Mar 22, 2022

Merged to maser, branch-3.3, branch-3.2 and branch-3.1.

kazuyukitanimura pushed a commit to kazuyukitanimura/spark that referenced this pull request Aug 10, 2022
…rException

### What changes were proposed in this pull request?

Added null check for `exec.metricValues`.

### Why are the changes needed?

When requesting Restful API  {baseURL}/api/v1/applications/$appId/sql/$executionId which is introduced by this PR apache#28208, it can cause NullPointerException. The root cause is, when calling method doUpdate() of `LiveExecutionData`, `metricsValues` can be null. Then, when statement `printableMetrics(graph.allNodes, exec.metricValues)` is executed, it will throw NullPointerException.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Tested manually.

Closes apache#35884 from yym1995/fix-npe.

Lead-authored-by: Yimin <[email protected]>
Co-authored-by: Yimin Yang <[email protected]>
Signed-off-by: Yuming Wang <[email protected]>
(cherry picked from commit 99992a4)
Signed-off-by: Yuming Wang <[email protected]>
(cherry picked from commit c3aace7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants