-
Notifications
You must be signed in to change notification settings - Fork 46
fix(api): add bulk_upsert method and unit tests for opensearch BaseOSDB class #990
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
base: main
Are you sure you want to change the base?
Changes from all commits
586d311
35dd659
fe52fc2
14f9515
cfb5e53
f22047f
0775e3d
0886112
497583a
0f15cd2
392c88b
0ac5dba
1c66216
48474ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,13 +4,14 @@ | |
| import json | ||
| import logging | ||
| from abc import ABCMeta, abstractmethod | ||
| from collections.abc import AsyncIterator | ||
| from collections.abc import AsyncIterator, Iterable | ||
| from contextvars import ContextVar | ||
| from datetime import datetime | ||
| from typing import Any, Self | ||
|
|
||
| from opensearchpy import AsyncOpenSearch | ||
| from opensearchpy.exceptions import RequestError | ||
| from opensearchpy.helpers import async_bulk | ||
|
|
||
| from diracx.core.exceptions import DocumentUpsertError, InvalidQueryError | ||
| from diracx.core.extensions import DiracEntryPoint, select_from_extension | ||
|
|
@@ -213,6 +214,35 @@ async def upsert(self, vo: str, doc_id: int, document: Any) -> None: | |
| response, | ||
| ) | ||
|
|
||
| async def bulk_upsert( | ||
| self, | ||
| documents: Iterable[tuple[str, int, dict[str, Any]]], | ||
| ) -> tuple[int, list[Any]]: | ||
| """Bulk upsert documents.""" | ||
| actions = ( | ||
| { | ||
| "_op_type": "update", | ||
| "_index": self.index_name(vo, doc_id), | ||
| "_id": doc_id, | ||
| "doc": document, | ||
| "doc_as_upsert": True, | ||
| "retry_on_conflict": 10, | ||
| } | ||
| for vo, doc_id, document in documents | ||
| ) | ||
|
|
||
| success, errors = await async_bulk( | ||
| self.client, | ||
| actions, | ||
| raise_on_error=False, | ||
| raise_on_exception=False, | ||
|
aldbr marked this conversation as resolved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm just wondering what happens if there is a connection issue with the DB and no exception is raised.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also I am wondering whether it would make sense to use |
||
| ) | ||
|
|
||
| if errors: | ||
| logger.warning("Bulk upsert completed with %d errors", len(errors)) | ||
|
|
||
| return success, errors | ||
|
|
||
| async def search( | ||
| self, parameters, search, sorts, *, per_page: int = 100, page: int | None = None | ||
| ) -> list[dict[str, Any]]: | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment here |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| from __future__ import annotations | ||
|
|
||
| pytest_plugins = ["diracx.testing.mock_osdb"] |
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.
bulk_upsertwould need to overridden withinjob_parameters_dbbecause we are inserting aJobIDand atimestamp:diracx/diracx-db/src/diracx/db/os/job_parameters.py
Lines 37 to 43 in 202f84c
Here it would not work I think (and it looks like it's not spotted within the tests).
I actually wonder whether
upsertis useful now that we havebulk_upsert.I would suggest we just drop
upsertand replace it everywhere withbulk_upsert, what do you think?