You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openwhisk.apache.org by GitBox <gi...@apache.org> on 2022/04/28 06:33:18 UTC
[GitHub] [openwhisk] ningyougang opened a new pull request, #5228: Complete old activation when update action
ningyougang opened a new pull request, #5228:
URL: https://github.com/apache/openwhisk/pull/5228
<!--- Provide a concise summary of your changes in the Title -->
- [x] Complete old activation directly when update action if doesn't exist old container.
This is bug, if doesn't fix, the activation will be failed as below
```
activation processing is not initiated for 60000 ms
```
in this case, due to when update action, old activation already existed in old memoryQueue and no old container to fetch it,
so it is better to complete the old activation directly for safe.
- [x] Add exponential delay for `container creation retry`
## Description
<!--- Provide a detailed description of your changes. -->
<!--- Include details of what problem you are solving and how your changes are tested. -->
## Related issue and scope
<!--- Please include a link to a related issue if there is one. -->
- [ ] I opened an issue to propose and discuss this change (#????)
## My changes affect the following components
<!--- Select below all system components are affected by your change. -->
<!--- Enter an `x` in all applicable boxes. -->
- [ ] API
- [ ] Controller
- [ ] Message Bus (e.g., Kafka)
- [ ] Loadbalancer
- [x] Scheduler
- [ ] Invoker
- [ ] Intrinsic actions (e.g., sequences, conductors)
- [ ] Data stores (e.g., CouchDB)
- [ ] Tests
- [ ] Deployment
- [ ] CLI
- [ ] General tooling
- [ ] Documentation
## Types of changes
<!--- What types of changes does your code introduce? Use `x` in all the boxes that apply: -->
- [x] Bug fix (generally a non-breaking change which closes an issue).
- [x] Enhancement or new feature (adds new functionality).
- [ ] Breaking change (a bug fix or enhancement which changes existing behavior).
## Checklist:
<!--- Please review the points below which help you make sure you've covered all aspects of the change you're making. -->
- [ ] I signed an [Apache CLA](https://github.com/apache/openwhisk/blob/master/CONTRIBUTING.md).
- [ ] I reviewed the [style guides](https://github.com/apache/openwhisk/blob/master/CONTRIBUTING.md#coding-standards) and followed the recommendations (Travis CI will check :).
- [ ] I added tests to cover my changes.
- [ ] My changes require further changes to the documentation.
- [ ] I updated the documentation where necessary.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang merged pull request #5228: send old version memoryQueue's stale activation to queueManager when update action
Posted by GitBox <gi...@apache.org>.
ningyougang merged PR #5228:
URL: https://github.com/apache/openwhisk/pull/5228
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5228: send old version memoryQueue's stale activation to queueManager when update action
Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5228:
URL: https://github.com/apache/openwhisk/pull/5228#discussion_r867597667
##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala:
##########
@@ -829,6 +834,23 @@ class MemoryQueue(private val etcdClient: EtcdClient,
}
}
+
+ private def handleStaleActivationsWhenUpdateAction(queueManager: ActorRef): Unit = {
Review Comment:
Updated the method name accordingly.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] jiangpengcheng commented on a diff in pull request #5228: send old version memoryQueue's stale activation to queueManager when update action
Posted by GitBox <gi...@apache.org>.
jiangpengcheng commented on code in PR #5228:
URL: https://github.com/apache/openwhisk/pull/5228#discussion_r867313371
##########
tests/src/test/scala/org/apache/openwhisk/core/scheduler/queue/test/MemoryQueueFlowTests.scala:
##########
@@ -1496,6 +1497,7 @@ class MemoryQueueFlowTests
// queue should not be terminated as there is an activation
Thread.sleep(gracefulShutdownTimeout.toMillis)
+ fsm.underlyingActor.containers = Set(testContainerId)
Review Comment:
adding this line covers case that there is still a container for old queue, but does not cover case that there is no container for old queue
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5228: send old version memoryQueue's stale activation to queueManager when update action
Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5228:
URL: https://github.com/apache/openwhisk/pull/5228#discussion_r867329125
##########
tests/src/test/scala/org/apache/openwhisk/core/scheduler/queue/test/MemoryQueueFlowTests.scala:
##########
@@ -1534,20 +1536,14 @@ class MemoryQueueFlowTests
probe.expectTerminated(fsm, 10.seconds)
case _ =>
- // queue is stale and will be removed
+ fsm.underlyingActor.containers = Set(testContainerId)
parent.expectMsg(staleQueueRemovedMsg)
+ parent.expectMsg(message)
+ // queue is stale and will be removed
probe.expectMsg(Transition(fsm, state, Removing))
fsm ! QueueRemovedCompleted
- // queue should not be terminated as there is an activation
Review Comment:
Oh, fixed.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5228: Complete old activation when update action
Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5228:
URL: https://github.com/apache/openwhisk/pull/5228#discussion_r860534907
##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala:
##########
@@ -665,6 +679,9 @@ class MemoryQueue(private val etcdClient: EtcdClient,
goto(Removed) using NoData()
} else {
+ if (containers.size > 0) {
Review Comment:
If already has container for the remain activations, kill the monit actor to `don't create containers any more`
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5228: Complete old activation when update action
Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5228:
URL: https://github.com/apache/openwhisk/pull/5228#discussion_r860533606
##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/container/CreationJobManager.scala:
##########
@@ -97,7 +96,7 @@ class CreationJobManager(feedFactory: (ActorRefFactory, String, String, Int, Arr
if (retryCount >= retryLimit || !error.exists(ContainerCreationError.whiskErrors.contains)) {
logging.error(
this,
- s"[$creationId] Failed to create container $retryCount/$retryLimit times for $cause. Finished creation")
+ s"[$action] [$creationId] Failed to create container $retryCount/$retryLimit times for $cause. Finished creation")
Review Comment:
Make it easy to debug in future maybe.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5228: send old version memoryQueue's stale activation to queueManager when update action
Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5228:
URL: https://github.com/apache/openwhisk/pull/5228#discussion_r867329152
##########
tests/src/test/scala/org/apache/openwhisk/core/scheduler/queue/test/MemoryQueueFlowTests.scala:
##########
@@ -1496,6 +1497,7 @@ class MemoryQueueFlowTests
// queue should not be terminated as there is an activation
Thread.sleep(gracefulShutdownTimeout.toMillis)
+ fsm.underlyingActor.containers = Set(testContainerId)
Review Comment:
Updated accordingly.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5228: send old version memoryQueue's stale activation to queueManager when update action
Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5228:
URL: https://github.com/apache/openwhisk/pull/5228#discussion_r867598178
##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala:
##########
@@ -561,6 +561,20 @@ class MemoryQueue(private val etcdClient: EtcdClient,
// let QueueManager know this queue is no longer in charge.
context.parent ! staleQueueRemovedMsg
+ if (queue.size > 0) {
+ // if doesn't exist old container to pull old memoryQueue's activation, complete the activation directly
+ if (containers.size == 0) {
Review Comment:
Yes
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5228: send old version memoryQueue's stale activation to queueManager when update action
Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5228:
URL: https://github.com/apache/openwhisk/pull/5228#discussion_r860534907
##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala:
##########
@@ -665,6 +679,9 @@ class MemoryQueue(private val etcdClient: EtcdClient,
goto(Removed) using NoData()
} else {
+ if (containers.size > 0) {
Review Comment:
If already has container for the remain activations, kill the monit actor to `don't create containers any more`
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] jiangpengcheng commented on a diff in pull request #5228: send old version memoryQueue's stale activation to queueManager when update action
Posted by GitBox <gi...@apache.org>.
jiangpengcheng commented on code in PR #5228:
URL: https://github.com/apache/openwhisk/pull/5228#discussion_r867312939
##########
tests/src/test/scala/org/apache/openwhisk/core/scheduler/queue/test/MemoryQueueFlowTests.scala:
##########
@@ -1534,20 +1536,14 @@ class MemoryQueueFlowTests
probe.expectTerminated(fsm, 10.seconds)
case _ =>
- // queue is stale and will be removed
+ fsm.underlyingActor.containers = Set(testContainerId)
parent.expectMsg(staleQueueRemovedMsg)
+ parent.expectMsg(message)
+ // queue is stale and will be removed
probe.expectMsg(Transition(fsm, state, Removing))
fsm ! QueueRemovedCompleted
- // queue should not be terminated as there is an activation
Review Comment:
why these lines are removed? since there is a container in https://github.com/apache/openwhisk/pull/5228/files#diff-4b5788222d7949baf4e09b570e4fa49d3fa79e5432ff50c798b04353758c8d4aR1539, I think message should not be sent to `QueueManager`?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5228: send old version memoryQueue's stale activation to queueManager when update action
Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5228:
URL: https://github.com/apache/openwhisk/pull/5228#discussion_r865664944
##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala:
##########
@@ -561,6 +561,20 @@ class MemoryQueue(private val etcdClient: EtcdClient,
// let QueueManager know this queue is no longer in charge.
context.parent ! staleQueueRemovedMsg
+ if (queue.size > 0) {
+ // if doesn't exist old container to pull old memoryQueue's activation, complete the activation directly
+ if (containers.size == 0) {
Review Comment:
Regarding `I think we need to consider containers being created(in-progress containers).`
I think have no need to consider in-progress containers during action update in this case, because
* Normally, the in-progress containers will be failed due to version not matched: https://github.com/apache/openwhisk/blob/master/core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/ContainerMessageConsumer.scala#L85
* if above #L85 codes happens, has a option to create container using latest version of action, but if we do this, the new version container can't fetch the old version memoryQueue's stale activation.
So here, i think just consider `if (containers.size == 0)` is enough.
Regarding
```
And it's worth discussing but IIRC, in our downstream, we decided to promote the old version of activations to the latest one rather than just completing them with errors.
If we do this, I think QueueManager should act in the same way for stale activations.
```
when this case happens and have no existing old version container, just send the stale activation to queueManager to schedule to new version memoryQueue, and the activations will be fetched by new version container.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on pull request #5228: send old version memoryQueue's stale activation to queueManager when update action
Posted by GitBox <gi...@apache.org>.
ningyougang commented on PR #5228:
URL: https://github.com/apache/openwhisk/pull/5228#issuecomment-1120671778
I will merge this pr due to `this can make some test cases more stable`
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5228: send old version memoryQueue's stale activation to queueManager when update action
Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5228:
URL: https://github.com/apache/openwhisk/pull/5228#discussion_r867328992
##########
tests/src/test/scala/org/apache/openwhisk/core/scheduler/queue/test/MemoryQueueTests.scala:
##########
@@ -1343,7 +1429,9 @@ class MemoryQueueTests
Thread.sleep(1000)
memoryQueue.containers.size shouldBe 1
- memoryQueue.creationIds.size shouldBe 1
+ // the monit actor in memoryQueue may decide to create a container
+ memoryQueue.creationIds.size should be >= 1
+ memoryQueue.creationIds.size should be <= 2
Review Comment:
Due to this codes: https://github.com/apache/openwhisk/blob/master/core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala#L908
Above #L908 code is good due to `store the inprogressId to etcd needs some time`, so when containerCreationMessage created, need to add that creationId to creationIds.
This test case may not stable, e.g. due to the `creationId(testId1)` doesn't keep consistent with `CreationId.generate()`, if the monit actor decide to create a container very quickly, this test case would be failed.
So here, make it stable.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] style95 commented on a diff in pull request #5228: Complete old activation when update action
Posted by GitBox <gi...@apache.org>.
style95 commented on code in PR #5228:
URL: https://github.com/apache/openwhisk/pull/5228#discussion_r861641849
##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala:
##########
@@ -561,6 +561,20 @@ class MemoryQueue(private val etcdClient: EtcdClient,
// let QueueManager know this queue is no longer in charge.
context.parent ! staleQueueRemovedMsg
+ if (queue.size > 0) {
+ // if doesn't exist old container to pull old memoryQueue's activation, complete the activation directly
+ if (containers.size == 0) {
Review Comment:
I think we need to consider containers being created(in-progress containers).
And it's worth discussing but IIRC, in our downstream, we decided to promote the old version of activations to the latest one rather than just completing them with errors.
If we do this, I think QueueManager should act in the same way for stale activations.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] codecov-commenter commented on pull request #5228: Complete old activation when update action
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #5228:
URL: https://github.com/apache/openwhisk/pull/5228#issuecomment-1111970606
# [Codecov](https://codecov.io/gh/apache/openwhisk/pull/5228?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#5228](https://codecov.io/gh/apache/openwhisk/pull/5228?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3185af0) into [master](https://codecov.io/gh/apache/openwhisk/commit/3e3414ced7fb6e6d85b95f0f9cdd43a0a68da4fd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3e3414c) will **increase** coverage by `17.23%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #5228 +/- ##
===========================================
+ Coverage 44.49% 61.73% +17.23%
===========================================
Files 238 238
Lines 13957 13970 +13
Branches 570 572 +2
===========================================
+ Hits 6210 8624 +2414
+ Misses 7747 5346 -2401
```
| [Impacted Files](https://codecov.io/gh/apache/openwhisk/pull/5228?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../core/scheduler/container/CreationJobManager.scala](https://codecov.io/gh/apache/openwhisk/pull/5228/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9zY2hlZHVsZXIvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9vcGVud2hpc2svY29yZS9zY2hlZHVsZXIvY29udGFpbmVyL0NyZWF0aW9uSm9iTWFuYWdlci5zY2FsYQ==) | `0.00% <0.00%> (-83.34%)` | :arrow_down: |
| [...e/openwhisk/core/scheduler/queue/MemoryQueue.scala](https://codecov.io/gh/apache/openwhisk/pull/5228/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9zY2hlZHVsZXIvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9vcGVud2hpc2svY29yZS9zY2hlZHVsZXIvcXVldWUvTWVtb3J5UXVldWUuc2NhbGE=) | `0.00% <0.00%> (-83.74%)` | :arrow_down: |
| [...che/openwhisk/core/invoker/LogStoreCollector.scala](https://codecov.io/gh/apache/openwhisk/pull/5228/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9pbnZva2VyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvaW52b2tlci9Mb2dTdG9yZUNvbGxlY3Rvci5zY2FsYQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...hisk/core/scheduler/message/ContainerMessage.scala](https://codecov.io/gh/apache/openwhisk/pull/5228/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9zY2hlZHVsZXIvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9vcGVud2hpc2svY29yZS9zY2hlZHVsZXIvbWVzc2FnZS9Db250YWluZXJNZXNzYWdlLnNjYWxh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../openwhisk/core/scheduler/FPCSchedulerServer.scala](https://codecov.io/gh/apache/openwhisk/pull/5228/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9zY2hlZHVsZXIvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9vcGVud2hpc2svY29yZS9zY2hlZHVsZXIvRlBDU2NoZWR1bGVyU2VydmVyLnNjYWxh) | `0.00% <0.00%> (-95.84%)` | :arrow_down: |
| [...sk/core/scheduler/container/ContainerManager.scala](https://codecov.io/gh/apache/openwhisk/pull/5228/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9zY2hlZHVsZXIvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9vcGVud2hpc2svY29yZS9zY2hlZHVsZXIvY29udGFpbmVyL0NvbnRhaW5lck1hbmFnZXIuc2NhbGE=) | `0.00% <0.00%> (-88.72%)` | :arrow_down: |
| [...a/org/apache/openwhisk/http/BasicHttpService.scala](https://codecov.io/gh/apache/openwhisk/pull/5228/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2h0dHAvQmFzaWNIdHRwU2VydmljZS5zY2FsYQ==) | `3.17% <0.00%> (-87.31%)` | :arrow_down: |
| [...nwhisk/core/scheduler/queue/ContainerCounter.scala](https://codecov.io/gh/apache/openwhisk/pull/5228/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9zY2hlZHVsZXIvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9vcGVud2hpc2svY29yZS9zY2hlZHVsZXIvcXVldWUvQ29udGFpbmVyQ291bnRlci5zY2FsYQ==) | `0.00% <0.00%> (-84.22%)` | :arrow_down: |
| [...scheduler/queue/ElasticSearchDurationChecker.scala](https://codecov.io/gh/apache/openwhisk/pull/5228/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9zY2hlZHVsZXIvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9vcGVud2hpc2svY29yZS9zY2hlZHVsZXIvcXVldWUvRWxhc3RpY1NlYXJjaER1cmF0aW9uQ2hlY2tlci5zY2FsYQ==) | `0.00% <0.00%> (-80.77%)` | :arrow_down: |
| ... and [158 more](https://codecov.io/gh/apache/openwhisk/pull/5228/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/openwhisk/pull/5228?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/openwhisk/pull/5228?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [3e3414c...3185af0](https://codecov.io/gh/apache/openwhisk/pull/5228?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5228: send old version memoryQueue's stale activation to queueManager when update action
Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5228:
URL: https://github.com/apache/openwhisk/pull/5228#discussion_r865664944
##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala:
##########
@@ -561,6 +561,20 @@ class MemoryQueue(private val etcdClient: EtcdClient,
// let QueueManager know this queue is no longer in charge.
context.parent ! staleQueueRemovedMsg
+ if (queue.size > 0) {
+ // if doesn't exist old container to pull old memoryQueue's activation, complete the activation directly
+ if (containers.size == 0) {
Review Comment:
Regarding `I think we need to consider containers being created(in-progress containers).`
I think have no need to consider in-progress containers during action update in this case, because
* Normally, the in-progress containers will be failed due to version not matched: https://github.com/apache/openwhisk/blob/master/core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/ContainerMessageConsumer.scala#L85
* if above #L85 codes happens, has a option to create container using latest version of action, but if we do this, the new version container can't fetch the old version memoryQueue's stale activation as well.
So here, i think just consider `if (containers.size == 0)` is enough.
Regarding
```
And it's worth discussing but IIRC, in our downstream, we decided to promote the old version of activations to the latest one rather than just completing them with errors.
If we do this, I think QueueManager should act in the same way for stale activations.
```
when this case happens and have no existing old version container, just send the stale activation to queueManager to schedule to new version memoryQueue, and the activations will be fetched by new version container.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5228: send old version memoryQueue's stale activation to queueManager when update action
Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5228:
URL: https://github.com/apache/openwhisk/pull/5228#discussion_r867329361
##########
tests/src/test/scala/org/apache/openwhisk/core/scheduler/queue/test/QueueManagerTests.scala:
##########
@@ -554,7 +554,7 @@ class QueueManagerTests
mockConsumer,
QueueManagerConfig(maxRetriesToGetQueue = 2, maxSchedulingTime = 10 seconds)))
- queueManager ! activationMessage
+ queueManager ! activationMessage.copy(transid = TransactionId(messageTransId.meta.copy(start = Instant.now())))
Review Comment:
Make this test case more stable
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5228: send old version memoryQueue's stale activation to queueManager when update action
Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5228:
URL: https://github.com/apache/openwhisk/pull/5228#discussion_r867328992
##########
tests/src/test/scala/org/apache/openwhisk/core/scheduler/queue/test/MemoryQueueTests.scala:
##########
@@ -1343,7 +1429,9 @@ class MemoryQueueTests
Thread.sleep(1000)
memoryQueue.containers.size shouldBe 1
- memoryQueue.creationIds.size shouldBe 1
+ // the monit actor in memoryQueue may decide to create a container
+ memoryQueue.creationIds.size should be >= 1
+ memoryQueue.creationIds.size should be <= 2
Review Comment:
Due to this codes: https://github.com/apache/openwhisk/blob/master/core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala#L908
Above #L908 code is good due to `store the inprogressId to etcd needs some time`, so when containerCreationMessage created, need to add that creationId to creationIds in advance.
This test case may not stable, e.g. due to the `creationId(testId1)` doesn't keep consistent with `CreationId.generate()`, if the monit actor decide to create a container very quickly, this test case would be failed.
So here, make it stable.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] style95 commented on a diff in pull request #5228: send old version memoryQueue's stale activation to queueManager when update action
Posted by GitBox <gi...@apache.org>.
style95 commented on code in PR #5228:
URL: https://github.com/apache/openwhisk/pull/5228#discussion_r867577560
##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala:
##########
@@ -829,6 +834,23 @@ class MemoryQueue(private val etcdClient: EtcdClient,
}
}
+
+ private def handleStaleActivationsWhenUpdateAction(queueManager: ActorRef): Unit = {
Review Comment:
```suggestion
private def handleStaleActivationsWhenActionUpdated(queueManager: ActorRef): Unit = {
```
##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala:
##########
@@ -561,6 +561,20 @@ class MemoryQueue(private val etcdClient: EtcdClient,
// let QueueManager know this queue is no longer in charge.
context.parent ! staleQueueRemovedMsg
+ if (queue.size > 0) {
+ // if doesn't exist old container to pull old memoryQueue's activation, complete the activation directly
+ if (containers.size == 0) {
Review Comment:
ok, so this is to promote the activation revision by sending them to QueueManager.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org