This is an automated email from the ASF dual-hosted git repository. dubeejw pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/incubator-openwhisk.git
The following commit(s) were added to refs/heads/master by this push: new 1ed7568 Remove obsolete filter when determining container to remove. (#3130) 1ed7568 is described below commit 1ed7568800630059e12f849544af2734301f72b9 Author: rodric rabbah <rod...@gmail.com> AuthorDate: Sat Dec 23 20:47:51 2017 -0500 Remove obsolete filter when determining container to remove. (#3130) While collecting a container to remove, the filter checks the action and invocation namespace and excludes those from the candidate list. But this check is not necessary - any container in the free pool matching the action and namespace would be reused and the remove method should not be called in this case. --- .../whisk/core/containerpool/ContainerPool.scala | 22 +++++++++------------- .../containerpool/test/ContainerPoolTests.scala | 22 +++++----------------- 2 files changed, 14 insertions(+), 30 deletions(-) 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 5177026..1fa6b3a 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 25b17b6..a607f5b 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) } } -- To stop receiving notification emails like this one, please contact ['"commits@openwhisk.apache.org" <commits@openwhisk.apache.org>'].