You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openwhisk.apache.org by "style95 (via GitHub)" <gi...@apache.org> on 2023/03/30 01:03:40 UTC
[GitHub] [openwhisk] style95 opened a new pull request, #5391: Send a queue removed message to the queue manager
style95 opened a new pull request, #5391:
URL: https://github.com/apache/openwhisk/pull/5391
This is to handle the issue reported in https://github.com/apache/openwhisk/pull/5388 in another way.
<!--- Provide a concise summary of your changes in the Title -->
## 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
- [ ] 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: -->
- [ ] Bug fix (generally a non-breaking change which closes an issue).
- [ ] 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. -->
- [x] I signed an [Apache CLA](https://github.com/apache/openwhisk/blob/master/CONTRIBUTING.md).
- [x] 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] style95 commented on pull request #5391: Send a queue removed message to the queue manager
Posted by "style95 (via GitHub)" <gi...@apache.org>.
style95 commented on PR #5391:
URL: https://github.com/apache/openwhisk/pull/5391#issuecomment-1493497875
Once the queue manager receives a queue removed message, it will just remove the corresponding reference from queue pool and send an ack back to the sender.
If there is any logic to re-add the reference to the queue pool then there might be a race condition, but it always removes the reference and no race condition happens.
https://github.com/apache/openwhisk/blob/master/core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/QueueManager.scala#L204
One concern could be multiple acks sent from the queue manager, but it will also be handled by the wildcard case if there is no match for a certain state.
https://github.com/apache/openwhisk/blob/master/core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala#L670
--
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] bdoyle0182 commented on pull request #5391: Send a queue removed message to the queue manager
Posted by "bdoyle0182 (via GitHub)" <gi...@apache.org>.
bdoyle0182 commented on PR #5391:
URL: https://github.com/apache/openwhisk/pull/5391#issuecomment-1492402317
Will approve this so long as we're confident sending queue removed to the parent from these locations cannot create a new race condition.
--
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] bdoyle0182 commented on pull request #5391: Send a queue removed message to the queue manager
Posted by "bdoyle0182 (via GitHub)" <gi...@apache.org>.
bdoyle0182 commented on PR #5391:
URL: https://github.com/apache/openwhisk/pull/5391#issuecomment-1496317242
LGTM
--
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] bdoyle0182 commented on pull request #5391: Send a queue removed message to the queue manager
Posted by "bdoyle0182 (via GitHub)" <gi...@apache.org>.
bdoyle0182 commented on PR #5391:
URL: https://github.com/apache/openwhisk/pull/5391#issuecomment-1490796628
The edge case also occurs from this event
```
case Event(StateTimeout, _: NoActors) =>
logging.info(this, s"[$invocationNamespace:$action:$stateName] The queue is timed out, stop the queue.")
cleanUpDataAndGotoRemoved()
```
which this change wouldn't cover.
I think unit tests will need to be updated to make sure these changes don't have any side effects where sending queue removed to the parent occurs too early in the removal process
--
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] bdoyle0182 merged pull request #5391: Send a queue removed message to the queue manager
Posted by "bdoyle0182 (via GitHub)" <gi...@apache.org>.
bdoyle0182 merged PR #5391:
URL: https://github.com/apache/openwhisk/pull/5391
--
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 #5391: Send a queue removed message to the queue manager
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #5391:
URL: https://github.com/apache/openwhisk/pull/5391#issuecomment-1489561648
## [Codecov](https://codecov.io/gh/apache/openwhisk/pull/5391?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 [#5391](https://codecov.io/gh/apache/openwhisk/pull/5391?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c17372b) into [master](https://codecov.io/gh/apache/openwhisk/commit/60ca6605bb081f99906cff1a21caf75d47e414fa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (60ca660) will **decrease** coverage by `14.40%`.
> The diff coverage is `0.00%`.
> :exclamation: Current head c17372b differs from pull request most recent head ced1043. Consider uploading reports for the commit ced1043 to get more accurate results
```diff
@@ Coverage Diff @@
## master #5391 +/- ##
===========================================
- Coverage 76.91% 62.51% -14.40%
===========================================
Files 240 240
Lines 14588 14589 +1
Branches 629 618 -11
===========================================
- Hits 11221 9121 -2100
- Misses 3367 5468 +2101
```
| [Impacted Files](https://codecov.io/gh/apache/openwhisk/pull/5391?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...e/openwhisk/core/scheduler/queue/MemoryQueue.scala](https://codecov.io/gh/apache/openwhisk/pull/5391?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%> (-82.41%)` | :arrow_down: |
... and [75 files with indirect coverage changes](https://codecov.io/gh/apache/openwhisk/pull/5391/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?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] bdoyle0182 commented on pull request #5391: Send a queue removed message to the queue manager
Posted by "bdoyle0182 (via GitHub)" <gi...@apache.org>.
bdoyle0182 commented on PR #5391:
URL: https://github.com/apache/openwhisk/pull/5391#issuecomment-1492401381
Since this will be called from every shutdown case, I just want to make sure that there aren't any new race conditions created. Would it be possible for the queue removed message to be sent twice if it's called from here now and it was sent from another shutdown case such that the queue is removed from the manager map and then a new queue is created and this fsm sends another queue removed message removing it from the parent map after the new queue is created? I'm not sure if that could happen or matters, but want to check
--
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 pull request #5391: Send a queue removed message to the queue manager
Posted by "style95 (via GitHub)" <gi...@apache.org>.
style95 commented on PR #5391:
URL: https://github.com/apache/openwhisk/pull/5391#issuecomment-1491145388
@bdoyle0182
My bad, I added another missing part.
So both `cleanUpDataAndGotoRemoved()` and `cleanUpActorsAndGotoRemoved()` should send a queue removed message to the queue manager.
--
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] bdoyle0182 commented on pull request #5391: Send a queue removed message to the queue manager
Posted by "bdoyle0182 (via GitHub)" <gi...@apache.org>.
bdoyle0182 commented on PR #5391:
URL: https://github.com/apache/openwhisk/pull/5391#issuecomment-1490787969
This LGTM we need this as well but I think it's important to have the additional safeguard from the state timeout never occurring in the removing state creating an activation cycle such that the queue is never shut down so will still go ahead with merging that as well if you approve it.
--
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