You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by sarutak <gi...@git.apache.org> on 2014/09/17 22:09:49 UTC

[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

GitHub user sarutak opened a pull request:

    https://github.com/apache/spark/pull/2436

    [SPARK-3571] Spark standalone cluster mode doesn't work.

    I think, this issue is caused by #1106

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/sarutak/spark SPARK-3571

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/spark/pull/2436.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2436
    
----
commit 71e84b62bb6dcf57669dfccffb1de45123029f0d
Author: Kousuke Saruta <sa...@oss.nttdata.co.jp>
Date:   2014-09-17T20:06:18Z

    Modified Master to enable schedule normally

----


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2436#discussion_r17695194
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
    @@ -490,22 +490,27 @@ private[spark] class Master(
         // Randomization helps balance drivers
         val shuffledAliveWorkers = Random.shuffle(workers.toSeq.filter(_.state == WorkerState.ALIVE))
         val aliveWorkerNum = shuffledAliveWorkers.size
    -    var curPos = 0
    -    for (driver <- waitingDrivers.toList) { // iterate over a copy of waitingDrivers
    -      // We assign workers to each waiting driver in a round-robin fashion. For each driver, we
    -      // start from the last worker that was assigned a driver, and continue onwards until we have
    -      // explored all alive workers.
    -      curPos = (curPos + 1) % aliveWorkerNum
    -      val startPos = curPos
    -      var launched = false
    -      while (curPos != startPos && !launched) {
    -        val worker = shuffledAliveWorkers(curPos)
    -        if (worker.memoryFree >= driver.desc.mem && worker.coresFree >= driver.desc.cores) {
    -          launchDriver(worker, driver)
    -          waitingDrivers -= driver
    -          launched = true
    +
    +    if (aliveWorkerNum > 0) {
    +      var curPos = 0
    +      var stopPos = aliveWorkerNum
    --- End diff --
    
    Ah, yes. I'll modify.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2436#discussion_r17694410
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
    @@ -491,14 +491,13 @@ private[spark] class Master(
         val shuffledAliveWorkers = Random.shuffle(workers.toSeq.filter(_.state == WorkerState.ALIVE))
         val aliveWorkerNum = shuffledAliveWorkers.size
         var curPos = 0
    +    var stopPos = aliveWorkerNum
         for (driver <- waitingDrivers.toList) { // iterate over a copy of waitingDrivers
           // We assign workers to each waiting driver in a round-robin fashion. For each driver, we
           // start from the last worker that was assigned a driver, and continue onwards until we have
           // explored all alive workers.
    -      curPos = (curPos + 1) % aliveWorkerNum
    -      val startPos = curPos
    --- End diff --
    
    Wait, what do you mean? That's just another way of writing the same thing, and we can still start at `shuffledWorkers(curPos)` as before. Why is it less efficient?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2436#issuecomment-55971243
  
      [QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20491/consoleFull) for   PR 2436 at commit [`118c031`](https://github.com/apache/spark/commit/118c0316c7bf6340f3720c354d83607d19aaa8ed).
     * This patch **fails** unit tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2436#discussion_r17695987
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
    @@ -491,14 +491,13 @@ private[spark] class Master(
         val shuffledAliveWorkers = Random.shuffle(workers.toSeq.filter(_.state == WorkerState.ALIVE))
         val aliveWorkerNum = shuffledAliveWorkers.size
         var curPos = 0
    +    var stopPos = aliveWorkerNum
         for (driver <- waitingDrivers.toList) { // iterate over a copy of waitingDrivers
           // We assign workers to each waiting driver in a round-robin fashion. For each driver, we
           // start from the last worker that was assigned a driver, and continue onwards until we have
           // explored all alive workers.
    -      curPos = (curPos + 1) % aliveWorkerNum
    -      val startPos = curPos
    --- End diff --
    
    Yeah, I noticed and wrote same code.
    (2014/09/18 6:31), andrewor14 wrote:
    >
    > In core/src/main/scala/org/apache/spark/deploy/master/Master.scala:
    >
    > >      for (driver <- waitingDrivers.toList) { // iterate over a copy of waitingDrivers
    > >        // We assign workers to each waiting driver in a round-robin fashion. For each driver, we
    > >        // start from the last worker that was assigned a driver, and continue onwards until we have
    > >        // explored all alive workers.
    > > -      curPos = (curPos + 1) % aliveWorkerNum
    > > -      val startPos = curPos
    >
    > This is what I mean: 
    > andrewor14@apache:master...andrewor14:fix-standalone-cluster 
    > <https://github.com/andrewor14/spark/compare/apache:master...andrewor14:fix-standalone-cluster>
    >
    > —
    > Reply to this email directly or view it on GitHub 
    > <https://github.com/apache/spark/pull/2436/files#r17695766>.
    >


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:

    https://github.com/apache/spark/pull/2436#issuecomment-55975705
  
    retest this please


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2436#issuecomment-55953521
  
      [QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20484/consoleFull) for   PR 2436 at commit [`4817ecd`](https://github.com/apache/spark/commit/4817ecd958f3a7eff1674358bd3fe08be6d02f6a).
     * This patch merges cleanly.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on the pull request:

    https://github.com/apache/spark/pull/2436#issuecomment-55971553
  
    retest this please.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2436#issuecomment-55965053
  
      [QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20491/consoleFull) for   PR 2436 at commit [`118c031`](https://github.com/apache/spark/commit/118c0316c7bf6340f3720c354d83607d19aaa8ed).
     * This patch merges cleanly.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2436#discussion_r17692729
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
    @@ -491,14 +491,13 @@ private[spark] class Master(
         val shuffledAliveWorkers = Random.shuffle(workers.toSeq.filter(_.state == WorkerState.ALIVE))
         val aliveWorkerNum = shuffledAliveWorkers.size
         var curPos = 0
    +    var stopPos = aliveWorkerNum
         for (driver <- waitingDrivers.toList) { // iterate over a copy of waitingDrivers
           // We assign workers to each waiting driver in a round-robin fashion. For each driver, we
           // start from the last worker that was assigned a driver, and continue onwards until we have
           // explored all alive workers.
    -      curPos = (curPos + 1) % aliveWorkerNum
    -      val startPos = curPos
    --- End diff --
    
    I think the correct thing to do here is keep a counter that keeps track of how many workers we've looked at so far, then the while loop would look like:
    
    ```
    var numWorkersVisited = 0
    while (numWorkersVisited < numWorkersAlive && !launched) {
      ...
      numWorkersVisited += 1
    }
    ```


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2436#discussion_r17696251
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
    @@ -491,14 +491,13 @@ private[spark] class Master(
         val shuffledAliveWorkers = Random.shuffle(workers.toSeq.filter(_.state == WorkerState.ALIVE))
         val aliveWorkerNum = shuffledAliveWorkers.size
         var curPos = 0
    +    var stopPos = aliveWorkerNum
         for (driver <- waitingDrivers.toList) { // iterate over a copy of waitingDrivers
           // We assign workers to each waiting driver in a round-robin fashion. For each driver, we
           // start from the last worker that was assigned a driver, and continue onwards until we have
           // explored all alive workers.
    -      curPos = (curPos + 1) % aliveWorkerNum
    -      val startPos = curPos
    --- End diff --
    
    I had misunderstanding. I thought you mentioned remove stopPos and curPos. I understood keeping curPos.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2436#discussion_r17693786
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
    @@ -491,14 +491,13 @@ private[spark] class Master(
         val shuffledAliveWorkers = Random.shuffle(workers.toSeq.filter(_.state == WorkerState.ALIVE))
         val aliveWorkerNum = shuffledAliveWorkers.size
         var curPos = 0
    +    var stopPos = aliveWorkerNum
         for (driver <- waitingDrivers.toList) { // iterate over a copy of waitingDrivers
           // We assign workers to each waiting driver in a round-robin fashion. For each driver, we
           // start from the last worker that was assigned a driver, and continue onwards until we have
           // explored all alive workers.
    -      curPos = (curPos + 1) % aliveWorkerNum
    -      val startPos = curPos
    --- End diff --
    
    That's one way but if lots of driver waits, while loop have to start shuffleedWorkers(0) per one driver. I think it's not efficient.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:

    https://github.com/apache/spark/pull/2436#issuecomment-55975876
  
    Ok, LGTM... I tested this locally. Merging into master.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2436#issuecomment-55974434
  
      [QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20492/consoleFull) for   PR 2436 at commit [`7a4deea`](https://github.com/apache/spark/commit/7a4deea8552664cc53db1e20876b42631d36fae3).
     * This patch **passes** unit tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/spark/pull/2436


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2436#discussion_r17692494
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
    @@ -491,14 +491,13 @@ private[spark] class Master(
         val shuffledAliveWorkers = Random.shuffle(workers.toSeq.filter(_.state == WorkerState.ALIVE))
         val aliveWorkerNum = shuffledAliveWorkers.size
         var curPos = 0
    +    var stopPos = aliveWorkerNum
    --- End diff --
    
    Won't this result in an infinite loop if no workers are available? `curPos` is never equal to `aliveWorkerNum`


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2436#discussion_r17695015
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
    @@ -491,14 +491,13 @@ private[spark] class Master(
         val shuffledAliveWorkers = Random.shuffle(workers.toSeq.filter(_.state == WorkerState.ALIVE))
         val aliveWorkerNum = shuffledAliveWorkers.size
         var curPos = 0
    +    var stopPos = aliveWorkerNum
         for (driver <- waitingDrivers.toList) { // iterate over a copy of waitingDrivers
           // We assign workers to each waiting driver in a round-robin fashion. For each driver, we
           // start from the last worker that was assigned a driver, and continue onwards until we have
           // explored all alive workers.
    -      curPos = (curPos + 1) % aliveWorkerNum
    -      val startPos = curPos
    --- End diff --
    
    If we use numWorkersVisited and numWorkersAlive,  numWorkersVisited is set to 0 before entering while loop even though still in for loop.
    If it's still in for loop and last visited index is N, the index of a worker which is to be visited next should be N+1.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2436#discussion_r17695766
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
    @@ -491,14 +491,13 @@ private[spark] class Master(
         val shuffledAliveWorkers = Random.shuffle(workers.toSeq.filter(_.state == WorkerState.ALIVE))
         val aliveWorkerNum = shuffledAliveWorkers.size
         var curPos = 0
    +    var stopPos = aliveWorkerNum
         for (driver <- waitingDrivers.toList) { // iterate over a copy of waitingDrivers
           // We assign workers to each waiting driver in a round-robin fashion. For each driver, we
           // start from the last worker that was assigned a driver, and continue onwards until we have
           // explored all alive workers.
    -      curPos = (curPos + 1) % aliveWorkerNum
    -      val startPos = curPos
    --- End diff --
    
    This is what I mean: https://github.com/andrewor14/spark/compare/apache:master...andrewor14:fix-standalone-cluster


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2436#discussion_r17694510
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
    @@ -490,22 +490,27 @@ private[spark] class Master(
         // Randomization helps balance drivers
         val shuffledAliveWorkers = Random.shuffle(workers.toSeq.filter(_.state == WorkerState.ALIVE))
         val aliveWorkerNum = shuffledAliveWorkers.size
    -    var curPos = 0
    -    for (driver <- waitingDrivers.toList) { // iterate over a copy of waitingDrivers
    -      // We assign workers to each waiting driver in a round-robin fashion. For each driver, we
    -      // start from the last worker that was assigned a driver, and continue onwards until we have
    -      // explored all alive workers.
    -      curPos = (curPos + 1) % aliveWorkerNum
    -      val startPos = curPos
    -      var launched = false
    -      while (curPos != startPos && !launched) {
    -        val worker = shuffledAliveWorkers(curPos)
    -        if (worker.memoryFree >= driver.desc.mem && worker.coresFree >= driver.desc.cores) {
    -          launchDriver(worker, driver)
    -          waitingDrivers -= driver
    -          launched = true
    +
    +    if (aliveWorkerNum > 0) {
    +      var curPos = 0
    +      var stopPos = aliveWorkerNum
    --- End diff --
    
    As I mentioned before, this is not correct. `curPos != stopPos` will always be true since `curPos` is always mod `aliveWorkerNum`.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2436#issuecomment-55966901
  
      [QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20492/consoleFull) for   PR 2436 at commit [`7a4deea`](https://github.com/apache/spark/commit/7a4deea8552664cc53db1e20876b42631d36fae3).
     * This patch merges cleanly.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2436#discussion_r17695418
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
    @@ -491,14 +491,13 @@ private[spark] class Master(
         val shuffledAliveWorkers = Random.shuffle(workers.toSeq.filter(_.state == WorkerState.ALIVE))
         val aliveWorkerNum = shuffledAliveWorkers.size
         var curPos = 0
    +    var stopPos = aliveWorkerNum
         for (driver <- waitingDrivers.toList) { // iterate over a copy of waitingDrivers
           // We assign workers to each waiting driver in a round-robin fashion. For each driver, we
           // start from the last worker that was assigned a driver, and continue onwards until we have
           // explored all alive workers.
    -      curPos = (curPos + 1) % aliveWorkerNum
    -      val startPos = curPos
    --- End diff --
    
    I still don't understand what you mean. The behavior we want here is this: for each driver, we start from the position where the last driver left off, and we want to loop through each worker at most once (i.e. we don't want to exit the loop as soon as we have looked at `numWorkersAlive`). If the last driver's last visited index is N, then we still start from N + 1 because we keep track of the position `curPos`. `numWorkersVisited` is a counter, not an index into `shuffledWorkers`. Does that make sense?


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:

    https://github.com/apache/spark/pull/2436#issuecomment-55955314
  
    Oops, good catch...


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2436#issuecomment-55967262
  
      [QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20488/consoleFull) for   PR 2436 at commit [`4e51e35`](https://github.com/apache/spark/commit/4e51e3578fa7b351ea652c2371eecc27ff92aa01).
     * This patch **passes** unit tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2436#issuecomment-55957810
  
      [QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20488/consoleFull) for   PR 2436 at commit [`4e51e35`](https://github.com/apache/spark/commit/4e51e3578fa7b351ea652c2371eecc27ff92aa01).
     * This patch merges cleanly.


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:

    https://github.com/apache/spark/pull/2436#issuecomment-55962852
  
      [QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20484/consoleFull) for   PR 2436 at commit [`4817ecd`](https://github.com/apache/spark/commit/4817ecd958f3a7eff1674358bd3fe08be6d02f6a).
     * This patch **passes** unit tests.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `class JavaSparkContext(val sc: SparkContext)`
      * `  class ArrayConstructor extends net.razorvine.pickle.objects.ArrayConstructor `
      * `      throw new IllegalStateException("The main method in the given main class must be static")`
      * `class TaskCompletionListenerException(errorMessages: Seq[String]) extends Exception `
      * `class NonASCIICharacterChecker extends ScalariformChecker `
      * `        class Dummy(object):`
      * `class RatingDeserializer(FramedSerializer):`
      * `class SCCallSiteSync(object):`
      * `case class CreateTableAsSelect(`
      * `  class Encoder[T <: NativeType](columnType: NativeColumnType[T]) extends compression.Encoder[T] `
      * `  class Encoder[T <: NativeType](columnType: NativeColumnType[T]) extends compression.Encoder[T] `
      * `  class Encoder[T <: NativeType](columnType: NativeColumnType[T]) extends compression.Encoder[T] `
      * `  class Encoder extends compression.Encoder[IntegerType.type] `
      * `  class Decoder(buffer: ByteBuffer, columnType: NativeColumnType[IntegerType.type])`
      * `  class Encoder extends compression.Encoder[LongType.type] `
      * `  class Decoder(buffer: ByteBuffer, columnType: NativeColumnType[LongType.type])`
      * `case class CreateTableAsSelect(`
      * `class JavaStreamingContext(val ssc: StreamingContext) extends Closeable `



---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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


[GitHub] spark pull request: [SPARK-3571] Spark standalone cluster mode doe...

Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:

    https://github.com/apache/spark/pull/2436#discussion_r17692759
  
    --- Diff: core/src/main/scala/org/apache/spark/deploy/master/Master.scala ---
    @@ -491,14 +491,13 @@ private[spark] class Master(
         val shuffledAliveWorkers = Random.shuffle(workers.toSeq.filter(_.state == WorkerState.ALIVE))
         val aliveWorkerNum = shuffledAliveWorkers.size
         var curPos = 0
    +    var stopPos = aliveWorkerNum
         for (driver <- waitingDrivers.toList) { // iterate over a copy of waitingDrivers
           // We assign workers to each waiting driver in a round-robin fashion. For each driver, we
           // start from the last worker that was assigned a driver, and continue onwards until we have
           // explored all alive workers.
    -      curPos = (curPos + 1) % aliveWorkerNum
    -      val startPos = curPos
    --- End diff --
    
    Then you don't need `startPos` or `stopPos`


---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

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