Skip to content

feat: consolidator MVP follow ups#10

Merged
volmedo merged 7 commits into
mainfrom
vic/fix/consolidator-mvp-followups
Oct 8, 2025
Merged

feat: consolidator MVP follow ups#10
volmedo merged 7 commits into
mainfrom
vic/fix/consolidator-mvp-followups

Conversation

@volmedo

@volmedo volmedo commented Oct 7, 2025

Copy link
Copy Markdown
Contributor

This PR addresses most of the feedback that Alan left in #6 around the consolidation process.

The main change is that now the original egress/track invocations are stored in the DB. Besides, consolidation follows a regular ucanto flow. An egress/consolidate handler is registered in a ucanto server (a specific server for consolidation, separated from the public-facing one receiving egress/track invocations) and an egress/consolidate invocation is executed on each batch. The result of the process is an egress/consolidate receipt that contains the total egress accounted for along with any potential errors that might have happened during consolidation.

Endpoint string `dynamodbav:"endpoint"`
Cause []byte `dynamodbav:"cause"`
ReceivedAt string `dynamodbav:"receivedAt"`
Processed bool `dynamodbav:"proc"`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Processed bool `dynamodbav:"proc"`
ProcessedAt string `dynamodbav:"processedAt"`

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently there is this flag here and the processedAt field is in the consolidated record

@frrist frrist left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a question looks good otherwise

Comment on lines 178 to 180
if err := c.egressTable.MarkAsProcessed(ctx, records); err != nil {
return fmt.Errorf("marking records as processed: %w", err)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we intend to address this comment in this PR or a different one? #5 (comment)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, on a different one. Sorry it took me a minute to create the issue: #11

@volmedo volmedo merged commit 5236c8d into main Oct 8, 2025
4 of 5 checks passed
@volmedo volmedo deleted the vic/fix/consolidator-mvp-followups branch October 8, 2025 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants