[GitHub] [spark] squito commented on issue #26696: [WIP][SPARK-18886][CORE] Make locality wait time be the time since a TSM's available slots were fully utilized

[GitBox <gi...@apache.org>]

squito commented on issue #26696: [WIP][SPARK-18886][CORE] Make locality wait time be the time since a TSM's available slots were fully utilized
URL: https://github.com/apache/spark/pull/26696#issuecomment-564289018
 
 
   I'm not sure yet about resetting the timer.  Just writing down some thoughts -- it *would* be better to reset the timer if either (a) more executors have been added with better locality (which is very common with dynamic allocation, especially during initial "warmup" phase of a job which reads a large input) or (b) another stage / job has finished, and so now you've got access to local resources which used to be "pinned" to the higher priority stage.
   
   (a) feels like something we could address by only allowing resetting after `TaskSetManager.computeValidLocalityLevels()` changes the valid set.
   
   (b) I'm not sure ... you could detect a change in the total number of running tasksets, but you really want to do it even before prior tasksets have totally finished, but still have the last wave running.  
   
   I'm also not sure we should necessarily reset the timer even in these cases.  eg. you might only add one local executor out of 1k, is it really worth resetting?  Its certainly easier to understand what's happening if you never reset.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org