problem: no notifications for new targets (JSON, Pulsar)#26
Open
splix wants to merge 1 commit into
Open
Conversation
solution: extend the Notification struct with new types
There was a problem hiding this comment.
Pull request overview
Extends the archive notification payload to support non-file targets (JSON-per-field layouts and Pulsar streaming) by making location a typed object and emitting notifications based on writer-reported output locations.
Changes:
- Replaced
Notification.location: StringwithNotification.location: Location(tagged enum) and bumped schema version to/v2; addedNotificationBuilderfor constructing notifications from per-range locations. - Added
TargetFileWriter::locations()and implemented it across storage backends so per-height targets (JSON, Pulsar) can report one location per height. - Updated the archiver pipeline to collect and publish multiple notifications per processing step (one per reported location), with tests/docs updates.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/storage/pulsar.rs | Accumulates broker receipts per height and reports Location::Pulsar via locations(). |
| src/storage/objects.rs | Tracks the session Range in the writer and reports Location::File via locations(). |
| src/storage/mod.rs | Adds TargetFileWriter::locations() contract and imports Location. |
| src/storage/json_objects.rs | Tracks per-row produced JSON objects and reports per-height Location::Files. |
| src/storage/json_fs.rs | Tracks per-row produced JSON files and reports per-height Location::Files; adds tests for per-height notification behavior. |
| src/storage/fs.rs | Tracks the session Range in the writer and reports Location::File via locations(). |
| src/notify/pulsar.rs | Updates notifier test to use the new typed Location. |
| src/notify/mod.rs | Introduces Location-typed notification schema (v2) + NotificationBuilder. |
| src/notify/location.rs | New Location enum and supporting structs (FileGroup, MessageRef, etc.) with serde tagging and tests. |
| src/formats/json.rs | Adds FileSlot tagging to JSON field outputs so writers can build FileGroup without filename parsing. |
| src/archiver/table.rs | Switches tx/trace processing to generate multiple notifications from writer locations(). |
| src/archiver/order.rs | Updates test writers to satisfy the new locations() requirement. |
| src/archiver/mod.rs | Updates ProcessOutcome to carry Vec<Notification> instead of Option<Notification>. |
| src/archiver/block.rs | Switches block processing to generate multiple notifications from writer locations(). |
| src/archiver/archiver.rs | Publishes flattened notification streams (multiple per step) and updates helper extraction logic. |
| README.adoc | Documents notification v2 schema and the new location shapes (file, files, pulsar). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Coverage Report for CI Build 27293415880Coverage increased (+0.6%) to 49.306%Details
Uncovered Changes
Coverage Regressions20 previously-covered lines in 9 files lost coverage.
Coverage Stats
💛 - Coveralls |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
solution: extend the Notification struct with new types