-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Long running tasks: allow retrieving tasks by user #3268
✨ Long running tasks: allow retrieving tasks by user #3268
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3268 +/- ##
========================================
+ Coverage 82.3% 83.3% +1.0%
========================================
Files 755 682 -73
Lines 32249 30025 -2224
Branches 4121 3829 -292
========================================
- Hits 26549 25028 -1521
+ Misses 4859 4214 -645
+ Partials 841 783 -58
Flags with carried forward coverage won't be shown. Click here to find out more.
|
2e4096e
to
d18b721
Compare
e1f89d7
to
0f4b885
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
Some minor comments and thoughts below
|
||
|
||
def get_tasks_manager(app: web.Application) -> TasksManager: | ||
return app[APP_LONG_RUNNING_TASKS_MANAGER_KEY] | ||
|
||
|
||
def get_task_context(request: web.Request) -> dict[str, Any]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STYLE: the word task
might refer to different concepts. I suggest that we denote these as long_running_tasks
or long_tasks
or lgr_tasks
... or something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this not a bit exagerated? this lives in a module named long_running_tasks already. and everything in here talks about tasks. I do agree with that fact.
I will think of it for a different PR.
I could also think of renaming the module package into something like rest_async_task instead of long running... since what is long?
packages/service-library/src/servicelib/aiohttp/long_running_tasks/_dependencies.py
Show resolved
Hide resolved
packages/service-library/src/servicelib/aiohttp/long_running_tasks/_routes.py
Outdated
Show resolved
Hide resolved
packages/service-library/src/servicelib/fastapi/long_running_tasks/_routes.py
Outdated
Show resolved
Hide resolved
packages/service-library/tests/aiohttp/long_running_tasks/conftest.py
Outdated
Show resolved
Hide resolved
...ervice-library/tests/aiohttp/long_running_tasks/test_long_running_tasks_with_task_context.py
Outdated
Show resolved
Hide resolved
@@ -139,6 +141,8 @@ async def create_projects(request: web.Request): | |||
query_params=query_params, | |||
user_id=req_ctx.user_id, | |||
predefined_project=predefined_project, | |||
task_context=jsonable_encoder(req_ctx), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so here the lrtask context is decided to be exactly as the request context. Is it always the case?
THOUGHT:
if so, perhaps we need to store the request
(storage) always instead of defining a new task_context
...
AFAIK you want to keep track of the "original" request that triggered the lrtask, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, but how do I know what needs to be taken out of the request?
services/web/server/tests/unit/with_dbs/02/test_projects_cancellations.py
Outdated
Show resolved
Hide resolved
0f4b885
to
6f83874
Compare
properly url encoded
ee7b24b
to
b3939e8
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
What do these changes do?
This is a pre-requisite so that the frontend may retrieve currently running project copy tasks after a refresh or a snap (will need @odeimaiz to modify the frontend accordingly).
long running tasks framework:
{user_id: 123, product: osparc}
, then every task route will filter with that task context (e.g. user 1 cannot see the tasks of user 3, user 1 can only cancel his/her own tasks)webserver
GET /tasks
in the webserver REST API, which returns the list of currently running async tasks in the webserver for a specific user_id (through the usual logged in user facility): return the list of tasks of the user in the form ofPOST /v0/projects?from_study=de2578c5-431e-6257-a462-d7bf73b76c0c
Related issue/s
related to ITISFoundation/osparc-issues#428
How to test
Checklist