Skip to content

[SPARK-27012][core] Storage tab shows rdd details even after executor ended#23920

Closed
ajithme wants to merge 6 commits intoapache:masterfrom
ajithme:cachestorage
Closed

[SPARK-27012][core] Storage tab shows rdd details even after executor ended#23920
ajithme wants to merge 6 commits intoapache:masterfrom
ajithme:cachestorage

Conversation

@ajithme
Copy link
Contributor

@ajithme ajithme commented Feb 28, 2019

What changes were proposed in this pull request?

After we cache a table, we can see its details in Storage Tab of spark UI. If the executor has shutdown ( graceful shutdown/ Dynamic executor scenario) UI still shows the rdd as cached and when we click the link it throws error. This is because on executor remove event, we fail to adjust rdd partition details @ org.apache.spark.status.AppStatusListener#onExecutorRemoved

How was this patch tested?

Have tested this fix in UI manually
Edit: Added UT

@ajithme
Copy link
Contributor Author

ajithme commented Feb 28, 2019

any suggestions @cloud-fan @squito

@srowen
Copy link
Member

srowen commented Feb 28, 2019

You may have a point here, but what does the executor going away have to do with the object being cached, overall? that state is stored on the driver; partitions of data are across many executors.

@ajithme
Copy link
Contributor Author

ajithme commented Feb 28, 2019

The storage tab populates UI based on the rdd's available partitions. UI i provides a link to see how these partitions are distributed among executors which are alive. When executor goes away we should remove it from rdd available partition so that it doesn't show up in UI. right ?
The patch handles two scenarios

  1. All available partitions executors are lost
  2. Some of the partitions executors are lost

In case 1, we remove complete storage information
In case 2, we show the storage information, but only with available partitions(executors)

Copy link
Contributor

@vanzin vanzin left a comment

Choose a reason for hiding this comment

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

Needs unit test.

@ajithme
Copy link
Contributor Author

ajithme commented Mar 1, 2019

@vanzin Thanks for the review. I have updated the code as per your review comment. Do you see any more changes required.?

Copy link
Contributor

@attilapiros attilapiros left a comment

Choose a reason for hiding this comment

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

I suggest testing the case where the second executor is also lost (in the existing test method).

@ajithme
Copy link
Contributor Author

ajithme commented Mar 2, 2019

@attilapiros @vanzin @srowen Thanks for the inputs. I have updated accordingly

@vanzin
Copy link
Contributor

vanzin commented Mar 4, 2019

ok to test

@ajithme
Copy link
Contributor Author

ajithme commented Mar 5, 2019

@vanzin I have updated the PR as per the comments

@SparkQA
Copy link

SparkQA commented Mar 5, 2019

Test build #103008 has finished for PR 23920 at commit 1a1c89e.

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

@SparkQA
Copy link

SparkQA commented Mar 5, 2019

Test build #103018 has finished for PR 23920 at commit 32377da.

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

@vanzin vanzin changed the title [SPARK-27012] Storage tab shows rdd details even after executor ended [SPARK-27012][core] Storage tab shows rdd details even after executor ended Mar 5, 2019
@vanzin
Copy link
Contributor

vanzin commented Mar 5, 2019

Merging to master / 2.4 / 2.3 (if no conflicts).

@vanzin
Copy link
Contributor

vanzin commented Mar 5, 2019

Doesn't merge to 2.4; if you'd like the fix there, please open a separate PR.

@vanzin vanzin closed this in 6207360 Mar 5, 2019
@ajithme
Copy link
Contributor Author

ajithme commented Mar 5, 2019

will create a separate PR for 2.4 branch. Thanks @vanzin @attilapiros @srowen

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