Add aggregated Postgres writer for status change topic#189
Add aggregated Postgres writer for status change topic#189kevinwallimann wants to merge 13 commits into
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughAdds Postgres aggregation for ChangesStatus Change Aggregation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant EventGate
participant WriterPostgres
participant Postgres
Client->>EventGate: Send status_change event
EventGate->>WriterPostgres: Dispatch TOPIC_STATUS_CHANGE
WriterPostgres->>WriterPostgres: Map event and serialize context
WriterPostgres->>Postgres: Execute upsert_status_change
Postgres-->>WriterPostgres: Insert or update aggregated job row
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
a7f6b26 to
83fb752
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
adr/001-status-change-db/001-status-change-db.md (1)
51-59: 🩺 Stability & Availability | 🔵 TrivialVerify FK constraints don't break out-of-order parent-child event processing.
The ADR defines FK constraints on
parent_job_idandinitial_job_id, but also states events may be received out-of-order during failure scenarios. If a child job event arrives before its parent event, the FK constraint would reject the insert. The integration test schema intentionally omits these constraints, so this scenario is untested. Confirm whether parent-child event ordering is guaranteed in production, or whether the FK constraints should be deferred or removed.🤖 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 `@adr/001-status-change-db/001-status-change-db.md` around lines 51 - 59, The FK constraints on parent_job_id and initial_job_id in the ADR may conflict with out-of-order event arrival, so review the intended production ordering guarantees before keeping them as immediate constraints. Update the schema/design in the ADR to either state that parent events always exist first, or change the FK approach to deferred enforcement or remove the constraints; reference the job relationship constraints and the event processing flow described in the status-change DB design.
🤖 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.
Nitpick comments:
In `@adr/001-status-change-db/001-status-change-db.md`:
- Around line 51-59: The FK constraints on parent_job_id and initial_job_id in
the ADR may conflict with out-of-order event arrival, so review the intended
production ordering guarantees before keeping them as immediate constraints.
Update the schema/design in the ADR to either state that parent events always
exist first, or change the FK approach to deferred enforcement or remove the
constraints; reference the job relationship constraints and the event processing
flow described in the status-change DB design.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 548c66cc-bac2-47cc-949d-a932af83f3d8
📒 Files selected for processing (9)
DEVELOPER.mdadr/001-status-change-db/001-status-change-db.mdconf/access.jsonsrc/utils/constants.pysrc/writers/sql/inserts.sqlsrc/writers/writer_postgres.pytests/integration/conftest.pytests/integration/schemas/postgres_schema.pytests/integration/test_status_change_writer.py
| finished_at TIMESTAMPTZ, | ||
| last_updated_at TIMESTAMPTZ NOT NULL, | ||
|
|
||
| CONSTRAINT fk_job_parent |
There was a problem hiding this comment.
These are not implemented anywhere.
BTW, this project does not treat DB as a first class citizen. I think that someone just manually created tables on the DB. It cannot continue like this, I've created a ticket here: #201
There was a problem hiding this comment.
Perfect, thanks! I imagined something similar like in Atum-Service. Great to see that it already exists there
| platform_metadata JSONB, | ||
| input_arguments JSONB, | ||
| additional_context JSONB, | ||
| attempt_number INTEGER NOT NULL CHECK (attempt_number > 0), |
There was a problem hiding this comment.
| attempt_number INTEGER NOT NULL CHECK (attempt_number > 0), | |
| attempt_number INTEGER NOT NULL DEFAULT 1 CHECK (attempt_number > 0), |
just for the sake of consistency
| ); | ||
|
|
||
| -- Table for test_status_change_writer | ||
| CREATE TABLE IF NOT EXISTS public_cps_za_status_change_aggregated_job ( |
There was a problem hiding this comment.
[you can ignore]
I've actually never liked the decision to have all tables within the common public schema & together. But I don't want to migrate now as it's low value and not plenty of good reasons to change it, so be it.
There was a problem hiding this comment.
I have no strong feelings about it, whatever you prefer
| started_at: datetime | None = None | ||
| finished_at: datetime | None = None | ||
|
|
||
| if event_type == "JobCreatedEvent": |
There was a problem hiding this comment.
what about JobUpdatedEvent ? For that one, these would be set to None here:
"created_at": created_at,
"started_at": started_at,
"finished_at": finished_at,and later in the SQL, I see COALESCE and it would just keep the old values. I feel that it's functionally correct, but please add the comment at least here?
There was a problem hiding this comment.
The normal order of events is
1 ) JobCreatedEvent
2 ) JobStartedEvent
3 ) Zero or more JobUpdatedEvents
n ) JobFinishedEvent
Step 1 and 2 can alternatively be merged in one JobCreatedAndStartedEvent
created_atis defined as the timestamp whenJobCreatedEventorJobCreatedAndStartedEventarrivedstarted_atis defined as the timestamp whenJobStartedEventorJobCreatedAndStartedEventarrivedfinished_atis defined as the timestamp whenJobFinishedEventarrived
For JobUpdatedEvent, there would be last_updated_at, but last_updated_at is used for all events, so there is no dedicated else block here for JobUpdatedEvent
If I would set created_at, started_at in the JobUpdatedEvent, it would overwrite the created_at from the JobCreatedEvent, which is not desirable.
Does that clarify?
| finished_at = CASE | ||
| WHEN EXCLUDED.last_updated_at >= t.last_updated_at | ||
| THEN COALESCE(EXCLUDED.finished_at, t.finished_at) | ||
| ELSE t.finished_at |
There was a problem hiding this comment.
I'm thinking if this is not 'losing data' and more robust implementation can be like this:
ELSE COALESCE(t.finished_at, EXCLUDED.finished_at)
--
Imagine, that there are JobUpdatedEvent and JobFinishedEvent arrivals.
JobUpdatedEvent updates last_updated_at to value 100 (for simplicity here).
Then JobFinishedEvent arrives but has last_updated_at = 99 and that causes the finished_at to be NULL, because the condition EXCLUDED.last_updated_at >= t.last_updated_at will match the ELSE brach and that will keep the old record, which is NULL because the JobUpdatedEvent wouldn't carry this information about the job finish timestamp.
This is an edge case scenario, when the events arrive slightly out of order - the last_updated_at computed by producer has some redelivery/clock issiue. So out of order events would be corrupted by this, eventually leading to data loss (finished_at not being updated / present at all)
Btw, this aligns finished_at with the "take latest non-null" strategy declared in the ADR and with how created_at/started_at already behave.
There was a problem hiding this comment.
Yes, I will fix it, because it should indeed be the "take latest non-null" merge strategy according to the ADR.
I didn't cover the edge case you're describing. In the current implementation, a producer clock issue is out of scope for the out-of-order handling, as it relies on the timestamp to determine the true order.
The out-of-order handling is intended to account for reorderings introduced by the queue systems between the producer system and this Lambda, as they may not guarantee total order delivery.
Of course, it would be possible to determine the correct order by the assumed order of events, i.e. Created > Started > Updated > Finished, and for multiple Updated events, one could rely on the timestamp as a fallback.
Do you think we should cover that edge case?
Co-authored-by: Ladislav Sulak <ladislav.sulak@absa.africa>
2 participants
Overview
Release Notes
Related
Closes #188
Summary by CodeRabbit