Conversation
26d26a4 to
6aeb36b
Compare
|
Quick note on Antoniu's inversion concern: the PoC already clamps schedule_order to be monotonic per dependence chain when limiter is enabled (schedule_order = max(base+defer, last_scheduled+1µs)). This prevents TXb from being scheduled earlier than TXa even if delay shrinks later.\n\nCaveat: the clamp state is in-memory per replica; it resets on restart and doesn’t coordinate across replicas. If we need strict monotonicity across restarts/replicas we’d need to persist last_scheduled per chain (bigger change). |
6aeb36b to
86b8688
Compare
86b8688 to
44472f0
Compare
|
Next step: implement a minimal PoC that uses DB as source of truth for dependency limit! ETA tmr |
🧪 CI InsightsHere's what we observed from your CI run for ffab6b1. 🟢 All jobs passed!But CI Insights is watching 👀 |
|
@claude /pr-review |
|
Claude finished @Eikix's task in 7m 27s —— View job PR Review Complete
SummaryOverall this is a well-designed feature that addresses a real problem (schedule_order inversions across host-listener instances). The DB-backed monotonic clamp is a minimal and effective solution. Issues Found
Bug DetailsType mismatch in
The Positive Observations
| |
There was a problem hiding this comment.
[AUTOMATED]
Code Review Summary
Overall this is a well-designed feature that addresses a real problem (schedule_order inversions across host-listener instances). The DB-backed monotonic clamp is a minimal and effective solution.
Issues Found
1 bug requiring attention before merge.
Positive Observations
- Clean token bucket implementation for rate limiting
- Good use of Prometheus metrics for observability
- Unit tests cover the limiter logic
- Documentation in README is helpful
- Graceful degradation when limiter is disabled (rate=0)
| @@ -0,0 +1,4 @@ | |||
| CREATE TABLE IF NOT EXISTS dependence_chain_schedule ( | |||
| dependence_chain_id bytea PRIMARY KEY, | |||
| last_scheduled_at TIMESTAMPTZ NOT NULL | |||
There was a problem hiding this comment.
[AUTOMATED] Bug: Type mismatch between TIMESTAMPTZ column and PrimitiveDateTime return type
The column last_scheduled_at is defined as TIMESTAMPTZ, but the Rust function clamp_schedule_order_db() returns PrimitiveDateTime. In sqlx with the time crate:
TIMESTAMP(without timezone) maps toPrimitiveDateTimeTIMESTAMPTZ(with timezone) maps toOffsetDateTime
The RETURNING last_scheduled_at clause will return a TIMESTAMPTZ value, and sqlx will fail at runtime when trying to decode it into PrimitiveDateTime.
Evidence: The existing schedule_order columns in the codebase use TIMESTAMP (not TIMESTAMPTZ):
-- 20250703000000_add_schedule_order_column.sql
ADD COLUMN IF NOT EXISTS schedule_order TIMESTAMP NOT NULL DEFAULT NOW();Suggested fix: Change TIMESTAMPTZ to TIMESTAMP for consistency with existing schedule_order columns:
CREATE TABLE IF NOT EXISTS dependence_chain_schedule (
dependence_chain_id bytea PRIMARY KEY,
last_scheduled_at TIMESTAMP NOT NULL
);Confidence: 92/100
|
Closing, in favour of #1907 which replaces it |
What
Why
How
Impact / load
Risks / mitigations
Why this is minimal
Tracking
Testing