-
-
Notifications
You must be signed in to change notification settings - Fork 5
feat(py-client): Implement "many" api for batch requests #546
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
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 |
|---|---|---|
|
|
@@ -2,11 +2,11 @@ | |
|
|
||
| import math | ||
| import warnings | ||
| from collections.abc import Mapping, Sequence | ||
| from collections.abc import Iterable, Mapping, Sequence | ||
| from dataclasses import asdict, dataclass | ||
| from datetime import UTC, datetime, timedelta | ||
| from io import BytesIO | ||
| from typing import IO, Any, Literal, NamedTuple, cast | ||
| from typing import IO, TYPE_CHECKING, Any, Literal, NamedTuple, cast | ||
| from urllib.parse import urlparse | ||
|
|
||
| import sentry_sdk | ||
|
|
@@ -39,6 +39,12 @@ | |
| PARAM_AUTH = "os_auth" | ||
|
|
||
|
|
||
| if TYPE_CHECKING: | ||
| from concurrent.futures import Executor | ||
|
|
||
| from objectstore_client.many import Operation, OperationResults | ||
|
|
||
|
|
||
| class GetResponse(NamedTuple): | ||
| metadata: Metadata | ||
| payload: IO[bytes] | ||
|
|
@@ -288,6 +294,73 @@ def _make_url(self, key: str | None, full: bool = False) -> str: | |
| return f"{self._base_url()}{path}" | ||
| return path | ||
|
|
||
| def _make_batch_url(self) -> str: | ||
| relative_path = f"/v1/objects:batch/{self._usecase.name}/{self._scope}/" | ||
| return self._base_path.rstrip("/") + relative_path | ||
|
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. Batch URL skips path encodingMedium Severity
Reviewed by Cursor Bugbot for commit c481164. Configure here. |
||
|
|
||
| def many( | ||
| self, | ||
| operations: Iterable[Operation], | ||
| *, | ||
| concurrency: int | None = None, | ||
| executor: Executor | None = None, | ||
| ) -> OperationResults: | ||
| """ | ||
| Executes multiple operations, batching them where possible. | ||
|
|
||
| Operations that satisfy the batch protocol's per-part size limit of 1MB | ||
| are grouped into batch requests to reduce network overhead. Inserts with | ||
| larger sizes (or unknown sizes) are sent as individual requests instead. | ||
|
|
||
| Args: | ||
| operations: The operations to execute, as | ||
| :class:`~objectstore_client.many.Get`, | ||
| :class:`~objectstore_client.many.Put`, | ||
| :class:`~objectstore_client.many.Delete`, and | ||
| :class:`~objectstore_client.many.Head` instances. Any iterable | ||
| works. | ||
| concurrency: The maximum number of requests in flight. Defaults to | ||
| ``1``, which runs everything sequentially on the calling thread, | ||
| without a thread pool. Raising it is sufficient to send requests | ||
| concurrently, but each request opens/closes its own connection | ||
| without additional configuration on the :class:`Client` (a | ||
| ``maxsize`` key in the ``connection_kwargs`` dict to control | ||
| ``urllib3`` connection pool size). | ||
|
Comment on lines
+322
to
+328
Member
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. In the Rust client, by default, we send the requests with some degree of concurrency. |
||
| executor: An executor to run requests on, instead of a thread pool | ||
| owned by this call. ``concurrency`` still caps how much work is | ||
| submitted to it at a time, and passing an executor overrides the | ||
| sequential behavior of ``concurrency=1``. | ||
|
|
||
| Returns: | ||
| An :class:`~objectstore_client.many.OperationResults` iterator over | ||
| the results. Results are yielded as responses come in, in no | ||
| particular order; each carries the ``index`` of the operation it | ||
| belongs to, which is the only handle on a keyless | ||
| :class:`~objectstore_client.many.Put`. This iterator is lazy, and if | ||
| it's abandoned without being fully consumed then operations that | ||
| haven't been dispatched are cancelled. | ||
|
|
||
| Raises: | ||
| ValueError: If ``concurrency`` is less than ``1``. | ||
|
|
||
| Example:: | ||
|
|
||
| from objectstore_client import many | ||
|
|
||
| results = session.many([many.Put(b"hello", key="k1"), many.Get("k2")]) | ||
| for result in results: | ||
| if result.error is not None: | ||
| ... # this operation failed | ||
| elif isinstance(result, many.GetResult): | ||
| ... # `result.response` is None if the object does not exist | ||
| """ | ||
| # Imported lazily to avoid a circular import at module load time. | ||
| from objectstore_client.many import execute_many | ||
|
|
||
| return execute_many( | ||
| self, operations, concurrency=concurrency, executor=executor | ||
| ) | ||
|
|
||
| def _make_multipart_url( | ||
| self, | ||
| action: str | 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.
I don't think there's a need to separate operations on the same key into different batches, as the result will be non-deterministic anyways when concurrency > 1.
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.
this idea was taken from #419, except that PR only applied it when
concurrency=1. this PR applies it for all values ofconcurrency. if the splitting logic is written anyway, why not apply it on all paths to at least make the race less likely?as for whether it's necessary: tbh i'm ambivalent. if i were using the objectstore client, i'd probably take care to keep only the last PUT/DELETE operation (last write wins) and drop any post-write GET/HEAD operations because we already know what they'll return (unless clients expect an out-of-band racing write operation lol). but there is a case one may want to express with a single
.many()today that would need this fix: aGET /foo/barto read the current value before aPUT /foo/barto replace the value. a non-atomic swap.this fix and
concurrency=1are needed for that case to work right with a singlesession.many()call. but as a workaround you can do asession.many()with all of yourGETs and then a secondsession.many()with all thePUTs. that eliminates any racing (as far as this client goes) and doesn't even requireconcurrency=1. it might be better to do it that way anyway.so i think i've talked myself into agreeing with you. @jan-auer is there anything else to consider about this or shall i remove this behavior?