Skip to content
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.

Sign up for GitHub

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

fix the PollingBatch with the current version of yagna #302

Merged
merged 10 commits into from
Apr 7, 2021
Merged

fix the PollingBatch with the current version of yagna #302

Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0665865
fix the PollingBatch with the current version of yagna
shadeofblue Mar 29, 2021
3e4b0a8
black, additional comment
shadeofblue Mar 29, 2021
3524e5b
- ","
shadeofblue Mar 29, 2021
cab9f94
use aiohttp's general `_request_timeout`
shadeofblue Mar 30, 2021
5a776a6
Merge branch 'master' into blue/polling-batch-timeout-argument
shadeofblue Mar 30, 2021
f851124
black...
shadeofblue Mar 30, 2021
942f46b
Merge branch 'blue/polling-batch-timeout-argument' of github.com:gole…
shadeofblue Mar 30, 2021
d6e5a35
Set maximum client-side timeout to 5 seconds, continue on client-side…
filipgolem Apr 7, 2021
474565b
Decrease timeout
filipgolem Apr 7, 2021
554abcc
Merge branch 'master' into blue/polling-batch-timeout-argument
shadeofblue Apr 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions yapapi/rest/activity.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,11 +167,13 @@ async def __aiter__(self) -> AsyncIterator[events.CommandEventContext]:
raise BatchTimeoutError()
try:
results: List[yaa.ExeScriptCommandResult] = await self._api.get_exec_batch_results(
self._activity_id, self._batch_id, timeout=timeout
self._activity_id, self._batch_id, _request_timeout=min(timeout, 5)
)
except asyncio.TimeoutError:
continue
except ApiException as err:
Copy link
Contributor

@azawlocki azawlocki Apr 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to ApiException we should also catch asyncio.TimeoutError here and raise BatchTimeoutError instead.

Copy link
Contributor

@azawlocki azawlocki Apr 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also shorten the amount of sleep when no new results are available, at the end of this method. It's now 10 seconds:

yapapi/yapapi/rest/activity.py

Lines 196 to 198 in 157e69e

if not any_new:
delay = min(10, max(0, self.seconds_left()))
await asyncio.sleep(delay)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's three seconds in yajsapi.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to ApiException we should also catch asyncio.TimeoutError here and raise BatchTimeoutError instead.

To avoid (tcp) connection closed errors, I limited maximum timeout to 5 seconds and added continue when timeouts happen.

if err.status == 408:
raise BatchTimeoutError()
continue
raise
any_new: bool = False
results = results[last_idx:]
Expand All @@ -194,7 +196,7 @@ async def __aiter__(self) -> AsyncIterator[events.CommandEventContext]:
if result.is_batch_finished:
break
if not any_new:
delay = min(10, max(0, self.seconds_left()))
delay = min(3, max(0, self.seconds_left()))
await asyncio.sleep(delay)


Expand Down