-
Notifications
You must be signed in to change notification settings - Fork 13
fix: Allow for ValidationErrors to lead to failed events #73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| PATH | ||
| remote: . | ||
| specs: | ||
| journaled (6.2.7) | ||
| journaled (6.2.8) | ||
| activejob | ||
| activerecord | ||
| activesupport | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| PATH | ||
| remote: .. | ||
| specs: | ||
| journaled (6.2.7) | ||
| journaled (6.2.8) | ||
| activejob | ||
| activerecord | ||
| activesupport | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| PATH | ||
| remote: .. | ||
| specs: | ||
| journaled (6.2.7) | ||
| journaled (6.2.8) | ||
| activejob | ||
| activerecord | ||
| activesupport | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ module Outbox | |
| # 4. Start workers: bundle exec rake journaled_worker:work | ||
| class Adapter < Journaled::DeliveryAdapter | ||
| class TableNotFoundError < StandardError; end | ||
| class RecordTooLargeError < StandardError; end | ||
|
|
||
| # Delivers events by inserting them into the database | ||
| # | ||
|
|
@@ -29,6 +30,15 @@ def self.deliver(events:, **) | |
| # Exclude the application-level id - the database will generate its own using uuid_generate_v7() | ||
| event_data = event.journaled_attributes.except(:id) | ||
|
|
||
| # The DB-generated id adds bytes to the JSON payload at send time, so a | ||
| # placeholder id keeps this estimate honest. | ||
| payload_bytesize = event_data.merge(id: SecureRandom.uuid).to_json.bytesize | ||
| if payload_bytesize > KinesisBatchSender::KINESIS_MAX_RECORD_BYTES | ||
| raise RecordTooLargeError, "Journaled event '#{event.journaled_attributes[:event_type]}' " \ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, re: @samandmoore's point:
I guess I'm sort of 🤞 that we would detect most case before it gets to a production request. But even then, I guess we have two choices:
I'm less a fan of option 2 because I think we ultimately want to do away with the DLQ -- it's a bit of a crutch that makes it easier for us to ignore / tighten the ratchet on guaranteeing deliverability from the moment we construct the event payload. |
||
| "exceeds Kinesis #{KinesisBatchSender::KINESIS_MAX_RECORD_BYTES}-byte per-record limit " \ | ||
| "(#{payload_bytesize} bytes); refusing to enqueue." | ||
| end | ||
|
Comment on lines
+35
to
+40
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need this here instead of as a validation on Do you think it's worth adding as a db constraint (albeit slightly less precise) instead?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. UUIDs are fixed-length, so it seems like we could maintain a reasonable length estimate (perhaps with some padding) in either place. (It all kinda comes down to the way the string serialization of the JSON works over in the outbox worker, right?)
I would say in addition to rather than instead -- the DB constraint can be a backstop that maintains the invariant, and the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And we don't strictly need the DB constraint as part of this PR, but if it's trivial to add then happy to review that too. But mainly I think it's useful to both validate the data in motion and then express the hard requirements in the schema, and both are valuable. |
||
|
|
||
| { | ||
| event_type: event.journaled_attributes[:event_type], | ||
| event_data:, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Journaled | ||
| VERSION = "6.2.7" | ||
| VERSION = "6.2.8" | ||
| end |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, and just flagging this for myself (if I ever try to reconstruct my understanding of the root cause), the
raise unlessis what essentially creates the poison pill jobs, as nothing upstream of this will mark these jobs as failed.