-
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
✨ Aiohttp long running tasks client #3290
✨ Aiohttp long running tasks client #3290
Conversation
f99512d
to
dcb708f
Compare
Codecov Report
@@ Coverage Diff @@
## master #3290 +/- ##
========================================
- Coverage 82.2% 81.3% -1.0%
========================================
Files 761 762 +1
Lines 32544 32629 +85
Branches 4160 4167 +7
========================================
- Hits 26781 26551 -230
- Misses 4903 5230 +327
+ Partials 860 848 -12
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Looking good. Please consider my request below.
packages/service-library/tests/aiohttp/long_running_tasks/test_long_running_tasks_client.py
Show resolved
Hide resolved
packages/service-library/src/servicelib/aiohttp/long_running_tasks/_server.py
Show resolved
Hide resolved
packages/service-library/src/servicelib/aiohttp/long_running_tasks/client.py
Outdated
Show resolved
Hide resolved
13afd5b
to
164e57d
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 useful feature!
I have some comments/thoughts regarding the design and a couple of questions
NOTE: I guess as soon as this client interface gets stable it will be also extended to fastapi-based servers, right?
packages/service-library/src/servicelib/aiohttp/long_running_tasks/client.py
Show resolved
Hide resolved
packages/service-library/src/servicelib/aiohttp/long_running_tasks/client.py
Outdated
Show resolved
Hide resolved
packages/service-library/src/servicelib/aiohttp/long_running_tasks/client.py
Outdated
Show resolved
Hide resolved
packages/service-library/src/servicelib/aiohttp/long_running_tasks/client.py
Show resolved
Hide resolved
packages/service-library/src/servicelib/aiohttp/long_running_tasks/client.py
Show resolved
Hide resolved
:yield: a task containing the current TaskProgress | ||
:rtype: Iterator[AsyncGenerator[LRTask, None]] | ||
""" | ||
task = None |
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, this function encapsulates logic with four possible requests, namely
_start
_waif_for_completion
_task_result
_abort_task
Each of the above can potentially raise ClientResponseError
, therefore the exceptions interface here remains ClientResonseError
. Q1: how does the caller know which of these requests above errored? Would it make sense to define an error wrapper that associates ClientResonseError
upon start
, while completion
, etc??
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.
hmm, I think the only where it might make sense to differentiate is the start. the rest shall behave as usual. as this is wrapped, I do not think the caller might know what is going on with the long running task internals.
packages/service-library/tests/aiohttp/long_running_tasks/test_long_running_tasks_client.py
Show resolved
Hide resolved
session: ClientSession, | ||
url: URL, | ||
json: Optional[RequestBody] = None, | ||
wait_timeout_s: int = 1 * _HOUR, |
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.
These timing options are related with the polling interval. I would rather reuse tenacity kwargs. This way the caller has far more flexibility in the way it pools (e.g. it might poll the state with an exponential/random delay etc)
I have some suggestions regarding the name as well below (check the test)
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.
no, the wait_timeout is related with how long the client wants to wait, a.k.a ClientTimeout. will rename it.
The interval one, I want to change that later by using a Retry-After header. potentially different from the one of tenacity
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, I changed these. please have a look.
I kept the client_timeout which makes sense I think
Kudos, SonarCloud Quality Gate passed!
|
What do these changes do?
pre-requisite to simplify usage in ✨ Webserver reports copy progress #3287
Related issue/s
related to ITISFoundation/osparc-issues#558
How to test
Checklist