Conversation
When logging in with an http:// URL against a remote server, reqwest silently downgrades POST to GET on 301/302 redirects (per RFC 7231), causing subsequent commands like `tower teams switch` to fail with 404.
…249) * [TOW-1836] Upgrade iceberg version and dependancies to latest stable * set versions as minimums
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughNormalize persisted tower URLs (convert http→https for non-local hosts), switch Polars Iceberg scans to a pyiceberg-backed reader, add configurable retry-and-refresh loops for Iceberg table inserts/deletes, bump workspace/python package versions and iceberg-related dependency constraints, and add concurrency tests for retry behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant TableAPI as Table.insert()/delete()
participant Iceberg as IcebergTable (append/delete)
participant Catalog as Catalog.refresh()
Client->>TableAPI: call insert/delete(data, max_retries, delay)
TableAPI->>Iceberg: attempt append/delete
alt CommitFailedException
Iceberg-->>TableAPI: raises CommitFailedException
TableAPI->>Catalog: refresh()
TableAPI->>TableAPI: sleep(retry_delay_seconds)
TableAPI->>Iceberg: retry append/delete (repeat up to max_retries)
else Success
Iceberg-->>TableAPI: success (commit)
end
TableAPI-->>Client: return final result or raise last exception
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
* Add commit retries to insert and delete table writes Parallel writes to the same Iceberg table can fail with CommitFailedException when the branch changes during active writes. This adds the same retry-with-refresh logic that upsert already has to both insert() and delete(), resolving concurrent write conflicts automatically. * Address PR #250 feedback: validate retry args and make tests deterministic Add input validation for max_retries and retry_delay_seconds across insert(), upsert(), and delete() to prevent confusing errors from negative values. Make concurrent insert/delete tests deterministic by forcing CommitFailedException on the first attempt rather than relying on real concurrency conflicts.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyproject.toml (1)
7-7:⚠️ Potential issue | 🟡 MinorVersion mismatch:
0.3.57vs PR titlev0.3.58.The project version in
pyproject.tomlis0.3.57, but the PR title indicates this is a release forv0.3.58. Consider bumping the version to match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` at line 7, The project version in pyproject.toml is still set to version = "0.3.57" but the PR title indicates release v0.3.58; update the version string in pyproject.toml to "0.3.58" (i.e., change the value of the version key) so the package metadata matches the intended release.
🧹 Nitpick comments (1)
src/tower/_tables.py (1)
29-30: Remove unusedrandomimport.The
randommodule is imported but never used in this file.🧹 Proposed fix
import time -import random🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tower/_tables.py` around lines 29 - 30, Remove the unused import by deleting the `random` import statement from the top of the module (currently `import random`) so only used modules like `time` remain; update any import grouping or formatting if needed and run linters to confirm no other unused imports remain in the file (`src/tower/_tables.py`).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pyproject.toml`:
- Line 7: The project version in pyproject.toml is still set to version =
"0.3.57" but the PR title indicates release v0.3.58; update the version string
in pyproject.toml to "0.3.58" (i.e., change the value of the version key) so the
package metadata matches the intended release.
---
Nitpick comments:
In `@src/tower/_tables.py`:
- Around line 29-30: Remove the unused import by deleting the `random` import
statement from the top of the module (currently `import random`) so only used
modules like `time` remain; update any import grouping or formatting if needed
and run linters to confirm no other unused imports remain in the file
(`src/tower/_tables.py`).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3816023e-e898-4074-97db-268e50a6195b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/tower-cmd/src/session.rspyproject.tomlsrc/tower/_tables.pytests/tower/test_tables.py
* Version bump to 0.3.58 * uv lock
Summary by CodeRabbit
New Features
Improvements
Tests
Chores