-
Notifications
You must be signed in to change notification settings - Fork 27
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
Infrastructure for DB trigger event receiver #644
Conversation
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.
Do we need this separate table, or can we simply use the queue?
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.
No, you can't use the queue because it doesn't provide catch-up state.
Also, I want rid of the queue table, I think.
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.
A more thorough explanation.
All the event receivers I've looked at need state about how far through the source they've gotten. cron is keeping the time, kafka has topic offsets*, db triggers may need the table timestamp also (depending on the application). This table is for recording that. It is not always possible or efficient to consult the workflow status table to rebuild this. The queue table is getting gc'd so I'd not want to rely on that either.
*I predict that we will eventually learn the limitations of letting the kafka side keep these, and want to take control over it... I've seen the ending of that movie before.
return knex.schema.withSchema('dbos') | ||
.createTable('event_dispatch_kv', function(table) { | ||
table.text('service_name').notNullable(); | ||
table.text('workflow_fn_name').notNullable(); |
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.
need to make sure the workflow name contains the class name
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.
Looks good! It makes a lot more sense now that there's a real implementation in the scheduler and the old scheduler state is gone.
Added the following things to Event Receiver API:
. Ability to query app (user) DB
. Ability to listen for notifications on app DB
. Ability to store state in the system DB