-
Notifications
You must be signed in to change notification settings - Fork 28
Create database for single run triggers #128
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
base: master
Are you sure you want to change the base?
Create database for single run triggers #128
Conversation
| VALUES(?, ?, ?) | ||
| """.stripMargin, | ||
| ) | ||
| prepared.setString(1, fireInstanceId) |
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.
Should we have a check that this id fits in the column?
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.
The job_name and job_group columns don't have checks on the other job_history or trigger_history tables. Since their columns are VARCHAR(100) NOT NULL, seemed appropriate to use the same for the single_run_trigger_id column. This is definitely something that could be added in a followup PR, but seems outside this PR's scope
worker/src/main/scala/com/lucidchart/piezo/SingleRunTriggerModel.scala
Outdated
Show resolved
Hide resolved
worker/src/main/scala/com/lucidchart/piezo/SingleRunTriggerModel.scala
Outdated
Show resolved
Hide resolved
worker/src/main/scala/com/lucidchart/piezo/SingleRunTriggerModel.scala
Outdated
Show resolved
Hide resolved
worker/src/main/scala/com/lucidchart/piezo/SingleRunTriggerModel.scala
Outdated
Show resolved
Hide resolved
worker/src/main/scala/com/lucidchart/piezo/SingleRunTriggerModel.scala
Outdated
Show resolved
Hide resolved
b44be52 to
78d57ed
Compare
a7c8451 to
3aba153
Compare
| if (scheduler.checkExists(jobKey)) { | ||
| try { | ||
| // Only run a trigger, if we haven't seen this id before | ||
| if (singleRunTriggerModel.hasJobAlreadyRun(id, jobKey)) { |
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.
Can you help me understand why we can't use existing tables for this? Don't we already keep a trigger history? Could we look to see if the job has ever been triggered before? Or even more specific could we have a new trigger we create each time and then just check if that fired (the later would support us triggering a "one time" job more than once if needed)
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.
The existing job_history and trigger_history tables gets cleaned up periodically by the JobHistoryCleanup.scala . We need to keep a permanent record of "one time" jobs that have triggered, hence why the new table was created
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 about updating the JobHistoryCleanup.scala job to exclude these one-time job triggers from its queries? We could have the trigger group or trigger name have something specific that we can filter on.
If we take this approach it means that all of our jobs still go through the same flows and we don't have to create a new database table (which is a bit of a pain).
If we go this route the changes would be:
-Updating the JobHistoryCleanup job and the JobHistoryModel and TriggerHistoryModel classes to exclude a certain trigger group name or trigger name prefix.
-Update piezo sync to configure one time jobs with this trigger group name or trigger name prefix
-Update piezo sync to create a new trigger if the last trigger for the job doesn't match what it expects.
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.
Yeah, that could be done. I don't have a preference for either, but avoiding creating a new table during the release process is nice
| WHERE | ||
| job_name=? | ||
| AND job_group=? | ||
| AND single_run_trigger_id=? |
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.
Where is the trigger id generated?
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.
It is provided to the route as a string /triggers/:group/:name/onetime/:id. This allows users to specify their own IDs and determine when to run a 2nd one-time job just by changing the id
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.
We should restrict what the trigger id can be then since it'll be in the path (we don't want spaces for example)
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.
Correct. It'll need to be URL-compliant
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.
It'll need to be URL-compliant
that isn't enough. We also want it to be short enough to fit inside the column. And being in the url it can still have spaces (encoded using %20).
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 would like to implement UNIX timestamps for the route, so how about we restrict it to a Long?
hunterrees
left a comment
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'm not ready to approve yet, but if we keep this approach I don't think we'll need changes. I'd like to discuss the option I outlined of not creating a new table before we proceed.
| if (scheduler.checkExists(jobKey)) { | ||
| try { | ||
| // Only run a trigger, if we haven't seen this id before | ||
| if (singleRunTriggerModel.hasJobAlreadyRun(id, jobKey)) { |
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 about updating the JobHistoryCleanup.scala job to exclude these one-time job triggers from its queries? We could have the trigger group or trigger name have something specific that we can filter on.
If we take this approach it means that all of our jobs still go through the same flows and we don't have to create a new database table (which is a bit of a pain).
If we go this route the changes would be:
-Updating the JobHistoryCleanup job and the JobHistoryModel and TriggerHistoryModel classes to exclude a certain trigger group name or trigger name prefix.
-Update piezo sync to configure one time jobs with this trigger group name or trigger name prefix
-Update piezo sync to create a new trigger if the last trigger for the job doesn't match what it expects.
| WHERE | ||
| job_name=? | ||
| AND job_group=? | ||
| AND single_run_trigger_id=? |
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.
We should restrict what the trigger id can be then since it'll be in the path (we don't want spaces for example)
2d1f1c2 to
cd63ac2
Compare
cd63ac2 to
22e8a05
Compare
This creates the
/triggers/:group/:name/onetime/:idroute to allow for one-time triggers to be created that don't run on a cron schedule. One-time triggers a different from simple triggers, because they require an ID provided to the route, allowing them to be managed outside of piezo