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

    https://github.com/apache/spark/pull/6430#discussion_r31397987
  
    --- Diff: 
core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala ---
    @@ -150,6 +151,9 @@ private[spark] class ExecutorAllocationManager(
       // Metric source for ExecutorAllocationManager to expose internal status 
to MetricsSystem.
       val executorAllocationManagerSource = new ExecutorAllocationManagerSource
     
    +  // Flag to measure whether numExecutorTarget could be adjusted
    +  val numTargetExecutorAdjustable = new AtomicBoolean(false)
    --- End diff --
    
    Hi @andrewor14 , do we need to consider this condition `(1) the initial 
executors have all registered` to change the `initializing` to false?
    
    Previously in this patch we set `initializing` to false  when: 
    1. a stage is submitted.
    2. an executor is timeout due to idle (default is 60 secs)
    
    Now we add another condition, so possibly we may have such scenario: when 
all the initial executors are registered, but no stage is submitted and 
executor is not idle timeout. So the executors can have a chance to ramp down 
at this time window, I think this will introduce additional requesting when 
stage is later submitted, is this a intended behavior?


---
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.
---

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

Reply via email to