Github user andrewor14 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/42#discussion_r10366355
  
    --- Diff: core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala 
---
    @@ -978,6 +971,11 @@ class DAGScheduler(
           logDebug("Additional executor lost message for " + execId +
                    "(epoch " + currentEpoch + ")")
         }
    +    // Block manager master actor should not be null except during tests
    +    if (blockManagerMaster.driverActor != null) {
    +      val storageStatusList = blockManagerMaster.getStorageStatus
    +      
listenerBus.post(SparkListenerExecutorsStateChange(storageStatusList))
    --- End diff --
    
    I have given more thought to the SparkListenerExecutorsStateChange event 
and have come to the following conclusions.
    
    Having the event take in the storage status list is a little arbitrary in 
two ways - (1) Every ExecutorStateChange event is supposed to be triggered only 
by one executor, but we fetch (and log) the storage status of all executors. 
(2) The event doesn't really need to maintain the blocks map in StorageStatus. 
When new executors have joined, they start off with no blocks, so the map is 
empty anyway. When existing executors have failed, we don't care about their 
blocks any more, so the map holds little value to us.
    
    This leads to the proposal to refactor this event in the following way - 
Keep around just enough arguments to reconstruct a StorageStatus on the UI 
side, ignoring the blocks map, which could potentially be huge and expensive to 
log. This means the event will now hold a (blockManagerId, maxMem) that 
corresponds to the given executor.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to