Skip to content

Conversation

@sarutak
Copy link
Member

@sarutak sarutak commented Jan 5, 2021

What changes were proposed in this pull request?

This PR makes AppStatusListener update the peak memory metrics for each Executor on task end like other peak memory metrics (e.g, stage, executors in a stage).

Why are the changes needed?

When AppStatusListener#onExecutorMetricsUpdate is called, peak memory metrics for Executors, stages and executors in a stage are updated but currently, the metrics only for Executors are not updated on task end.

Does this PR introduce any user-facing change?

Yes. Executor peak memory metrics is updated more accurately.

How was this patch tested?

After I run a job with local-cluster[1,1,1024] and visited /api/v1/<appid>/executors, I confirmed peakExecutorMemory metrics is shown for an Executor even though the life time of each job is very short .
I also modify the json files for HistoryServerSuite.

@HyukjinKwon
Copy link
Member

cc @gengliangwang, @jiangxb1987 and @Ngone51 FYI

@github-actions github-actions bot added the CORE label Jan 5, 2021
@SparkQA
Copy link

SparkQA commented Jan 5, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38251/

@SparkQA
Copy link

SparkQA commented Jan 5, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38251/

@SparkQA
Copy link

SparkQA commented Jan 5, 2021

Test build #133662 has finished for PR 31029 at commit 46a488d.

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

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you, @sarutak .
Merged to master for Apache Spark 3.2.0.

@gengliangwang
Copy link
Member

@sarutak @dongjoon-hyun shall we port this to branch-3.1 as well?

@dongjoon-hyun
Copy link
Member

This is registered as an improvement for 3.2.0, @gengliangwang .

@dongjoon-hyun
Copy link
Member

cc @HyukjinKwon since he is the release manager.

@sarutak
Copy link
Member Author

sarutak commented Jan 20, 2021

Hmm, I registered this improvement for 3.2 but there is a similar change (#30858) merged into 3.1.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Jan 20, 2021

That was merged a month ago (Dec 20, 2020) before RC cutting.

@HyukjinKwon
Copy link
Member

I am fine with backporting improvements if it's clearly safe. For example, I did backport some changes like c3d8352 too.

@dongjoon-hyun
Copy link
Member

+1 for the release manager's decision for this specific commit.

@sarutak
Copy link
Member Author

sarutak commented Jan 20, 2021

O.K, I'll open a backport PR.

sarutak added a commit to sarutak/spark that referenced this pull request Jan 20, 2021
…ask end

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

This PR makes `AppStatusListener` update the peak memory metrics for each Executor on task end like other peak memory metrics (e.g, stage, executors in a stage).

### Why are the changes needed?

When `AppStatusListener#onExecutorMetricsUpdate` is called, peak memory metrics for Executors, stages and executors in a stage are updated but currently, the metrics only for Executors are not updated on task end.

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

Yes. Executor peak memory metrics is updated more accurately.

### How was this patch tested?

After I run a job with `local-cluster[1,1,1024]` and visited `/api/v1/<appid>/executors`, I confirmed `peakExecutorMemory` metrics is shown for an Executor even though the life time of each job is very short .
I also modify the json files for `HistoryServerSuite`.

Closes apache#31029 from sarutak/update-executor-metrics-on-taskend.

Authored-by: Kousuke Saruta <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
gengliangwang pushed a commit that referenced this pull request Jan 20, 2021
… on task end

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

This PR backports SPARK-34005 (#31029).
This PR makes `AppStatusListener` update the peak memory metrics for each Executor on task end like other peak memory metrics (e.g, stage, executors in a stage).

### Why are the changes needed?

When `AppStatusListener#onExecutorMetricsUpdate` is called, peak memory metrics for Executors, stages and executors in a stage are updated but currently, the metrics only for Executors are not updated on task end.

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

Yes. Executor peak memory metrics is updated more accurately.

### How was this patch tested?

After I run a job with `local-cluster[1,1,1024]` and visited `/api/v1/<appid>/executors`, I confirmed `peakExecutorMemory` metrics is shown for an Executor even though the life time of each job is very short .
I also modify the json files for `HistoryServerSuite`.

Closes #31261 from sarutak/SPARK-34005-branch-3.1.

Authored-by: Kousuke Saruta <[email protected]>
Signed-off-by: Gengliang Wang <[email protected]>
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.

5 participants