True minio seek - #169
Conversation
📝 WalkthroughWalkthroughMinIO object downloads now use a lock-protected ranged reader with optional buffering, seeking, ETag conditions, and version support. Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MinioBucket
participant MinioObjectStream
participant Minio
Client->>MinioBucket: get_object_stream(object_name)
MinioBucket->>MinioObjectStream: open configured stream
Client->>MinioObjectStream: seek and read
MinioObjectStream->>Minio: get_object(offset, length, If-Match)
Minio-->>Client: requested object bytes
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
python/tests/test_minio_bucket.py (1)
34-75: 🩺 Stability & Availability | 🔵 TrivialRun the complete test matrix from
python/.These additions depend on the configured MinIO service. Before merge, run both unit and integration suites with
python/as the working directory and confirm the suite meets the required runtime target.As per coding guidelines: “Working directory must be
python/before running tests” and “run both unit and integration tests before task completion.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/tests/test_minio_bucket.py` around lines 34 - 75, Run the complete test matrix from the python/ working directory, including both unit and integration suites against the configured MinIO service, and verify the combined suite meets the required runtime target before completing the change.Source: Coding guidelines
python/tests/bucket_tester.py (2)
156-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpose
readintoin the typed stream contract.The
# type: ignore[attr-defined]is needed even thoughIBucket.get_object_stream()documentsreadinto(). Addreadintoto theObjectStreamprotocol/type alias inpython/bucketbase/ibucket.py, then remove this suppression so static checks enforce the same API the test validates.As per coding guidelines: “Behavioral Contracts: ensure interface implementations truly fulfill the interface contract without special handling in client code.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/tests/bucket_tester.py` around lines 156 - 174, The typed ObjectStream contract does not expose the readinto method used by get_object_stream consumers. Update the ObjectStream protocol/type alias in ibucket.py to declare readinto, then remove the type: ignore[attr-defined] suppression from test_get_object_stream_is_seekable so static typing validates the interface directly.Source: Coding guidelines
156-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the failure boundaries, not just the launch path.
This shared test covers successful seeks only. Add deterministic checks for invalid
whence, negative resulting positions, EOF reads, and partialreadinto()results so every backend is tested against the edge behavior too.As per coding guidelines: “New code must have comprehensive unit tests covering all paths, edge cases, and error scenarios.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/tests/bucket_tester.py` around lines 156 - 174, Extend test_get_object_stream_is_seekable with deterministic assertions for invalid whence values and seeks producing negative positions, verifying the expected errors. Also cover reads at EOF and readinto() when fewer bytes remain than the destination, asserting the returned byte counts and contents while preserving the existing successful-seek checks.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/bucketbase/minio_bucket.py`:
- Around line 82-108: Update the read method to catch MinIO’s
precondition-failure S3Error from get_object and translate it into one of the
documented exceptions exposed by get_object_stream/IBucket.get_object_stream,
preserving response cleanup and existing read behavior for other outcomes. Add
coverage for an object being overwritten after the captured etag so the
concurrent-modification path verifies the translated exception.
- Around line 29-41: Update MinioBucket.get_object() to download directly with
the Minio client instead of constructing _MinioRangeReader and triggering
stat_object(). Keep _MinioRangeReader usage confined to get_object_stream(),
preserving the existing seekable-stream behavior there.
---
Nitpick comments:
In `@python/tests/bucket_tester.py`:
- Around line 156-174: The typed ObjectStream contract does not expose the
readinto method used by get_object_stream consumers. Update the ObjectStream
protocol/type alias in ibucket.py to declare readinto, then remove the type:
ignore[attr-defined] suppression from test_get_object_stream_is_seekable so
static typing validates the interface directly.
- Around line 156-174: Extend test_get_object_stream_is_seekable with
deterministic assertions for invalid whence values and seeks producing negative
positions, verifying the expected errors. Also cover reads at EOF and readinto()
when fewer bytes remain than the destination, asserting the returned byte counts
and contents while preserving the existing successful-seek checks.
In `@python/tests/test_minio_bucket.py`:
- Around line 34-75: Run the complete test matrix from the python/ working
directory, including both unit and integration suites against the configured
MinIO service, and verify the combined suite meets the required runtime target
before completing the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e514c9b-9bc4-4937-b057-4f7163363e38
⛔ Files ignored due to path filters (1)
python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
.gitignorepython/bucketbase/ibucket.pypython/bucketbase/minio_bucket.pypython/bucketbase/versioned_minio_bucket.pypython/pyproject.tomlpython/tests/bucket_tester.pypython/tests/test_append_only_fs_bucket.pypython/tests/test_backup_multi_bucket.pypython/tests/test_fs_bucket.pypython/tests/test_ibucket.pypython/tests/test_integrated_cached_immutable_bucket.pypython/tests/test_memory_bucket.pypython/tests/test_minio_bucket.pypython/tests/test_versioned_minio_bucket.py
| def __init__(self, minio_client: Minio, bucket_name: str, object_name: str, version_id: str | None = None) -> None: | ||
| super().__init__() | ||
| metadata = minio_client.stat_object(bucket_name, object_name, version_id=version_id) | ||
| self._minio_client = minio_client | ||
| self._bucket_name = bucket_name | ||
| self._object_name = object_name | ||
| self._version_id = version_id | ||
| if metadata.size is None: | ||
| raise IOError(f"Minio returned no size for {object_name}") | ||
| self._size = metadata.size | ||
| self._etag = metadata.etag | ||
| self._position = 0 | ||
| self._lock = RLock() |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
# Locate the target file and relevant symbols.
git ls-files 'python/bucketbase/minio_bucket.py'
ast-grep outline python/bucketbase/minio_bucket.py --view expanded || true
printf '\n--- search ---\n'
rg -n "def get_object|def get_object_stream|stat_object|get_object_stream\(" python/bucketbase/minio_bucket.py python/bucketbase -S
printf '\n--- file slice 1 ---\n'
sed -n '1,180p' python/bucketbase/minio_bucket.py
printf '\n--- file slice 2 ---\n'
sed -n '180,340p' python/bucketbase/minio_bucket.pyRepository: eSAMTrade/bucketbase
Length of output: 18913
🏁 Script executed:
set -euo pipefail
# Find all call sites that rely on MinioBucket reads to understand blast radius.
rg -n "\.get_object_stream\(|\.get_object\(" python -S
# Inspect the reported callers around the relevant lines.
for f in $(rg -l "get_object_stream\(|get_object\(" python -S); do
printf '\n=== %s ===\n' "$f"
sed -n '1,260p' "$f"
doneRepository: eSAMTrade/bucketbase
Length of output: 50377
Keep a direct get_object() fast path MinioBucket.get_object() now always routes through _MinioRangeReader, which does a stat_object() before the download. That turns the common full-read path into HEAD + GET instead of a single GET. Reserve the seekable reader for get_object_stream().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/bucketbase/minio_bucket.py` around lines 29 - 41, Update
MinioBucket.get_object() to download directly with the Minio client instead of
constructing _MinioRangeReader and triggering stat_object(). Keep
_MinioRangeReader usage confined to get_object_stream(), preserving the existing
seekable-stream behavior there.
| def read(self, size: int = -1) -> bytes: | ||
| self._check_closed() | ||
| with self._lock: | ||
| remaining = max(0, self._size - self._position) | ||
| length = remaining if size is None or size < 0 else min(size, remaining) | ||
| if length == 0: | ||
| return b"" | ||
|
|
||
| request_headers: DictType | None = {"If-Match": f'"{self._etag}"'} if self._etag else None | ||
| response = self._minio_client.get_object( | ||
| self._bucket_name, | ||
| self._object_name, | ||
| offset=self._position, | ||
| length=length, | ||
| request_headers=request_headers, | ||
| version_id=self._version_id, | ||
| ) | ||
| try: | ||
| data = response.read() | ||
| finally: | ||
| response.close() | ||
| response.release_conn() | ||
|
|
||
| if len(data) != length: | ||
| raise IOError(f"Expected {length} bytes from {self._object_name} at offset {self._position}, but received {len(data)}") | ||
| self._position += length | ||
| return data |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Concurrent-modification failures escape as raw, undocumented S3Error.
The If-Match header ties every ranged read() to the etag captured at stat_object() time. If the object is overwritten mid-stream, MinIO will 412 and minio.error.S3Error (PreconditionFailed) will bubble straight out of read() — uncaught here, and not one of the exceptions get_object_stream()/IBucket.get_object_stream docstring promises (only FileNotFoundError/ValueError are documented). No test exercises this path either. Worth catching it and translating to something documented (or at least covering it with a test) so callers aren't surprised by a raw S3 error leaking through a supposedly stable seek/read contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/bucketbase/minio_bucket.py` around lines 82 - 108, Update the read
method to catch MinIO’s precondition-failure S3Error from get_object and
translate it into one of the documented exceptions exposed by
get_object_stream/IBucket.get_object_stream, preserving response cleanup and
existing read behavior for other outcomes. Add coverage for an object being
overwritten after the captured etag so the concurrent-modification path verifies
the translated exception.
1 participant
Title
<type>:<short summary>Ticket / Task
<URL>What & Why
"Builds X so that Y can Z" – two sentences max.
Scope
Checklist (self-review)
See Section 5.
Risk / Impact
Screenshots / Logs / Benchmarks
(only if relevant)
Author Self-Review Checklist
pytest -q/mvn testclean locally.git revertwithout dependency hell.python/scripts/run_mypy.batlocally with no critical errorsResources: