[GitHub] dubeejw closed pull request #3130: Remove obsolete filter when determining container to remove.

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

dubeejw closed pull request #3130: Remove obsolete filter when determining container to remove.
URL: https://github.com/apache/incubator-openwhisk/pull/3130
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/core/invoker/src/main/scala/whisk/core/containerpool/ContainerPool.scala b/core/invoker/src/main/scala/whisk/core/containerpool/ContainerPool.scala
index 5177026fed..1fa6b3ae86 100644
--- a/core/invoker/src/main/scala/whisk/core/containerpool/ContainerPool.scala
+++ b/core/invoker/src/main/scala/whisk/core/containerpool/ContainerPool.scala
@@ -95,7 +95,7 @@ class ContainerPool(childFactory: ActorRefFactory => ActorRef,
           }
           .orElse {
             // Remove a container and create a new one for the given job
-            ContainerPool.remove(r.action, r.msg.user.namespace, freePool).map { toDelete =>
+            ContainerPool.remove(freePool).map { toDelete =>
               removeContainer(toDelete)
               takePrewarmContainer(r.action).getOrElse {
                 createContainer()
@@ -200,9 +200,9 @@ object ContainerPool {
    * @param idles a map of idle containers, awaiting work
    * @return a container if one found
    */
-  def schedule[A](action: ExecutableWhiskAction,
-                  invocationNamespace: EntityName,
-                  idles: Map[A, ContainerData]): Option[(A, ContainerData)] = {
+  protected[containerpool] def schedule[A](action: ExecutableWhiskAction,
+                                           invocationNamespace: EntityName,
+                                           idles: Map[A, ContainerData]): Option[(A, ContainerData)] = {
     idles.find {
       case (_, WarmedData(_, `invocationNamespace`, `action`, _)) => true
       case _                                                      => false
@@ -210,21 +210,17 @@ object ContainerPool {
   }
 
   /**
-   * Finds the best container to remove to make space for the job passed to run.
+   * Finds the oldest previously used container to remove to make space for the job passed to run.
    *
-   * Determines the least recently used Free container in the pool.
+   * NOTE: This method is never called to remove an action that is in the pool already,
+   * since this would be picked up earlier in the scheduler and the container reused.
    *
-   * @param action the action that wants to get a container
-   * @param invocationNamespace the namespace, that wants to run the action
    * @param pool a map of all free containers in the pool
    * @return a container to be removed iff found
    */
-  def remove[A](action: ExecutableWhiskAction,
-                invocationNamespace: EntityName,
-                pool: Map[A, ContainerData]): Option[A] = {
-    // Try to find a Free container that is initialized with any OTHER action
+  protected[containerpool] def remove[A](pool: Map[A, ContainerData]): Option[A] = {
     val freeContainers = pool.collect {
-      case (ref, w: WarmedData) if (w.action != action || w.invocationNamespace != invocationNamespace) => ref -> w
+      case (ref, w: WarmedData) => ref -> w
     }
 
     if (freeContainers.nonEmpty) {
diff --git a/tests/src/test/scala/whisk/core/containerpool/test/ContainerPoolTests.scala b/tests/src/test/scala/whisk/core/containerpool/test/ContainerPoolTests.scala
index 25b17b63dd..a607f5ba06 100644
--- a/tests/src/test/scala/whisk/core/containerpool/test/ContainerPoolTests.scala
+++ b/tests/src/test/scala/whisk/core/containerpool/test/ContainerPoolTests.scala
@@ -380,30 +380,18 @@ class ContainerPoolObjectTests extends FlatSpec with Matchers with MockFactory {
   behavior of "ContainerPool remove()"
 
   it should "not provide a container if pool is empty" in {
-    ContainerPool.remove(createAction(), standardNamespace, Map()) shouldBe None
+    ContainerPool.remove(Map()) shouldBe None
   }
 
   it should "not provide a container from busy pool with non-warm containers" in {
     val pool = Map('none -> noData(), 'pre -> preWarmedData())
-    ContainerPool.remove(createAction(), standardNamespace, pool) shouldBe None
+    ContainerPool.remove(pool) shouldBe None
   }
 
-  it should "not provide a container from pool with one single free container with the same action and namespace" in {
+  it should "provide a container from pool with one single free container" in {
     val data = warmedData()
     val pool = Map('warm -> data)
-
-    // same data --> no removal
-    ContainerPool.remove(data.action, data.invocationNamespace, pool) shouldBe None
-
-    // different action, same namespace --> remove
-    ContainerPool.remove(createAction(data.action.name + "butDifferent"), data.invocationNamespace, pool) shouldBe Some(
-      'warm)
-
-    // different namespace, same action --> remove
-    ContainerPool.remove(data.action, differentNamespace, pool) shouldBe Some('warm)
-
-    // different everything --> remove
-    ContainerPool.remove(createAction(data.action.name + "butDifferent"), differentNamespace, pool) shouldBe Some('warm)
+    ContainerPool.remove(pool) shouldBe Some('warm)
   }
 
   it should "provide oldest container from busy pool with multiple containers" in {
@@ -414,6 +402,6 @@ class ContainerPoolObjectTests extends FlatSpec with Matchers with MockFactory {
 
     val pool = Map('first -> first, 'second -> second, 'oldest -> oldest)
 
-    ContainerPool.remove(createAction(), standardNamespace, pool) shouldBe Some('oldest)
+    ContainerPool.remove(pool) shouldBe Some('oldest)
   }
 }


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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