-
Notifications
You must be signed in to change notification settings - Fork 147
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
Add support for overriding created_at timestamps for copy transform workflows #282
Add support for overriding created_at timestamps for copy transform workflows #282
Conversation
002e3e7
to
0bc9bef
Compare
0bc9bef
to
fa9cb36
Compare
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.
@Johnabell I see the reason for adding this feature and the code looks reasonable to me.
lib/event_store.ex
Outdated
@@ -527,9 +527,16 @@ defmodule EventStore do | |||
Snapshotter.delete_snapshot(conn, source_uuid, opts) | |||
end | |||
|
|||
@accepted_overrides [:created_at] |
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.
Perhaps created_at
could be override_created_at
to make the intent clear?
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.
What do you think about created_at_override
, just a little concerned that override_created_at
might sound more like something that expects a boolean
?
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.
I have renamed it created_at_override
, but happy rename it to your suggestion if you prefer.
…_to_stream function
d87829f
to
705a8e2
Compare
@@ -24,12 +24,17 @@ WITH | |||
<% end %> | |||
), | |||
stream AS ( | |||
<%= if stream_id do %> | |||
<%= cond do %> |
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.
This makes stream_id and created_at mutually exclusive. Not sure that that's a good idea?
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.
I couldn't think of a use case for both updating stream_version
and the created_at
. Do you think there is a reasonable use case for this? I am happy to update this file if you think we should support this.
We are looking at running a copy-transform on our event stream.
This involves reading all events out of one event store and filtering/transforming them before writing them to a new event store. To support this we would like to maintain the timestamps on the original events in both the events table and the streams table.
This PR make that possible by allowing the caller to override
created_at
when appending events to the event store.