[batch] Fix list batches query and test #13237
Conversation
|
The test that lists batches timed out. The main problem is the limit in the aioclient used by the test_batch tests was passing a string rather than an integer. I assumed downstream the function was passing an integer. Therefore, we were doing this: and not So the query was running forever and scanning all batches from the I also was missing a tag annotation on the queries, but that was not causing the timeout. |
| @@ -634,7 +634,11 @@ async def _query_batches(request, user: str, q: str, version: int, last_batch_id | |||
| async def get_batches_v1(request, userdata): # pylint: disable=unused-argument | |||
There was a problem hiding this comment.
We should fix the types so that we cannot make this mistake in the future. There's a couple things we need to do:
- add type annotations here:
request: web.Request(userdata is a little harder to type, but maybe justDict[str, str]for now?) - Fix
_handle_api_errorto properly use types. It needs to use ParamSpec (see also the ParamSpec docs). This seems to type correctly and also cause mypy errors where it should:
T = TypeVar('T')
P = ParamSpec('P')
async def _handle_api_error(f: Callable[P, Awaitable[T]], *args: P.args, **kwargs: P.kwargs) -> Optional[T]:
|
I can keep digging, but it looks like the ParamSpec feature isn't available in the Python version we're running. I'll see if there's a workaround, but I'd really not like to hold up this change anymore than needed. |
|
It looks like ParamSpec was added in 3.10 |
|
Can we use the |
|
This commit looks like it's going to succeed. |
| @@ -227,17 +240,17 @@ async def _handle_ui_error(session, f, *args, **kwargs): | |||
| raise | |||
|
|
|||
|
|
|||
| async def _handle_api_error(f, *args, **kwargs): | |||
| async def _handle_api_error(f: Callable[P, Awaitable[T]], *args: P.args, **kwargs: P.kwargs) -> Optional[T]: | |||
There was a problem hiding this comment.
This should be a separate PR, but these new types and all those assert ... is not None reveal another issue.
This function conflates two distinct pieces of functionality:
- Make functions returning no values idempotent
- Convert
BatchUserErrorinto an HTTPResponse.
I'm a bit unsure if BatchOperationAlreadyCompletedError is the right thing to use. It seems a bit confusing if I am at the delete billing project form, then press submit, then I lose my WiFi, then refresh the page (clicking OK when it says "do you want to resubmit this form"), then I get "this billing project already is deleted". I guess that tells me that the first request actually did make it through, but as a user, all that matters is that the BP doesn't exist anymore, right? If I mistype the name I'd get a 404, so I can't accidentally think I deleted a different BP.
Regardless, I think we should have _handle_api_error which lacks the first except and _handle_idempotence_and_api_error which calls _handle_api_error wrapped in a try-except.
|
Approved, but see my comment about _handle_api_error. |
|
test_combiner_run is still way too slow. |
|
Thank you! I agree these functions need to be rewritten. |
The test that lists batches timed out. The main problem is the limit in the aioclient used by the test_batch tests was passing a string rather than an integer. I assumed downstream the function was passing an integer. Therefore, we were doing this:
batch_id < "137"
and not batch_id < 137.
So the query was running forever and scanning all batches from the test user.
I also was missing a tag annotation on the queries, but that was not causing the timeout.