v1 Scheduled Jobs / Third Party Sync Migration #171
Conversation
mattwilshire
commented
Oct 23, 2024
mattwilshire
commented
- Adds the ability to create scheduled jobs (currently only for third party syncs) that run off of a cron schedule.
- Third party sync configurations are pulled from the application.properties and pushed to a new scheduled_job table.
- The ScheduledJobManager handles the creation and deletion of jobs.
- The job listener updates the status of each job pre and post execution.
- New endpoints for fetching, triggering and toggling scheduled jobs.
mattwilshire
requested review from
maallen,
byronantak,
garionpin and
DarKhaos
webapp/src/main/resources/db/migration/V71__Add_scheduled_jobs.sql
Outdated
Show resolved
Hide resolved
webapp/src/main/resources/db/migration/V71__Add_scheduled_jobs.sql
Outdated
Show resolved
Hide resolved
webapp/src/main/resources/db/migration/V71__Add_scheduled_jobs.sql
Outdated
Show resolved
Hide resolved
webapp/src/main/resources/db/migration/V71__Add_scheduled_jobs.sql
Outdated
Show resolved
Hide resolved
webapp/src/main/java/com/box/l10n/mojito/entity/ScheduledJob.java
Outdated
Show resolved
Hide resolved
webapp/src/main/java/com/box/l10n/mojito/entity/ScheduledJobStatusEntity.java
Outdated
Show resolved
Hide resolved
| JobKey jobKey = scheduledJobManager.getJobKey(scheduledJob); | ||
|
|
||
| try { | ||
| if (!scheduledJobManager.getScheduler().checkExists(jobKey)) |
There was a problem hiding this comment.
You get the scheduler 3 distinct times in this method, would it not be possible to store it in a variable and just reuse the result? Or does the scheduler vary a lot and you need to wait to the last possible minute to get it each time?
There was a problem hiding this comment.
It would require capturing the scheduler in a static variable if it hasn't been set yet as this a web service, I believe this way is fine and I haven't seen other web services cache objects.
| } | ||
| } | ||
|
|
||
| @RequestMapping(method = RequestMethod.POST, value = "/api/jobs/{id}/toggle") |
There was a problem hiding this comment.
Nit: toggle is a strange term for this. I get that you mean it disable and enable a job, but this name feels confusing
There was a problem hiding this comment.
Honestly I have a preference for separate endpoints for enable and disable because this current endpoint expects the called to know what the previous state was. This could make debugging very difficult.
Example: I called toggled on a job, but I used the wrong job's id. Now the output could either be that it is toggle or not toggled depending on its prior state, which we don't know since this was a mistake. It adds a layer of "what was it before? because that dictates what it is now". Where as the if the endpoint is /enable you know if the status code is 200, then it is enabled now. No confusion, no mental gymnastics required
There was a problem hiding this comment.
Sounds good, I'll change it to two separate endpoints!
There was a problem hiding this comment.
+1 to having idempotent APIs, i.e.: separate enable/disable here
webapp/src/main/java/com/box/l10n/mojito/rest/scheduledjob/ScheduledJobWS.java
Show resolved
Hide resolved
| @Autowired ServerConfig serverConfig; | ||
|
|
||
| @Value( | ||
| "${l10n.scheduledJobs.thirdPartySync.notifications.title:MOJITO | Third party sync failed for {repository}}") |
There was a problem hiding this comment.
Sorry, I am a bit of a loss here. I know that the ${} syntax loops in the application properties to get the value but how does the {repository} bit work? That's not received from app properties and there doesn't appear to be a variable in scope that it can read from...
There was a problem hiding this comment.
If you use ${} in the application properties it will evaluate it on startup, if it doesn't find the variable, the process will crash straight away (IIRC). Other approach is to use {0}, {1} but that relies on the message having those parameters which isn't flexible
See:
String title = StrSubstitutor.replace( notificationTitle, ImmutableMap.of("repository", scheduledJob.getRepository().getName()), "{", "}");
If {repository} is in the custom title it will be replaced with the repository name that did the third party sync.
| "${l10n.scheduledJobs.thirdPartySync.notifications.title:MOJITO | Third party sync failed for {repository}}") | ||
| String notificationTitle; | ||
|
|
||
| private ScheduledJob scheduledJob; |
There was a problem hiding this comment.
Also a possible misunderstanding but why do we need to store these variables as class level variables? Does the context not have the details to find them again? 🤔
I don't know this library but I'm mainly worried about side-effects.
There was a problem hiding this comment.
The notification title is pulled in from the application properties, you can have different PagerDuty titles for different environments. The ScheduledJob here is set on execution, we pull it out of the database on the execute method, when the success or failure method is called by the listener we can reference the job as its tied to the instance.
| @Override | ||
| public void execute(JobExecutionContext jobExecutionContext) throws JobExecutionException { | ||
| // Fetch the scheduled job and cast the properties | ||
| scheduledJob = scheduledJobRepository.findByJobKey(jobExecutionContext.getJobDetail().getKey()); |
There was a problem hiding this comment.
Should this not be the try as well?
What if the cast fails or if the key is not found (by some misfortune)
There was a problem hiding this comment.
The only way for this not to exist if there was some manual altering of the database, the manager creates a job for reach scheduled job so it must exist otherwise it would never even get to this execute method.
webapp/src/main/java/com/box/l10n/mojito/service/scheduledjob/jobs/ScheduledThirdPartySync.java
Show resolved
Hide resolved
| try { | ||
| pd.triggerIncident(scheduledJob.getId(), payload); | ||
| } catch (PagerDutyException e) { | ||
| logger.error( |
There was a problem hiding this comment.
You take a lot of time and effort to build up all those useful urls above and if the incident fails to trigger, then you just discard it?
Can we not log it too to save us the trouble of reverse engineering the links? (It might be included in the payload, I'm not sure. Just the thought which was triggered)
webapp/src/main/java/com/box/l10n/mojito/service/scheduledjob/ScheduledJobStatusRepository.java
Outdated
Show resolved
Hide resolved
webapp/src/main/java/com/box/l10n/mojito/entity/ScheduledJob.java
Outdated
Show resolved
Hide resolved
webapp/src/main/java/com/box/l10n/mojito/entity/ScheduledJob.java
Outdated
Show resolved
Hide resolved
webapp/src/main/resources/db/migration/V71__Add_scheduled_jobs.sql
Outdated
Show resolved
Hide resolved
webapp/src/main/resources/db/migration/V71__Add_scheduled_jobs.sql
Outdated
Show resolved
Hide resolved
webapp/src/main/resources/db/migration/V71__Add_scheduled_jobs.sql
Outdated
Show resolved
Hide resolved
webapp/src/main/resources/db/migration/V71__Add_scheduled_jobs.sql
Outdated
Show resolved
Hide resolved
webapp/src/main/java/com/box/l10n/mojito/entity/ScheduledJob.java
Outdated
Show resolved
Hide resolved
webapp/src/main/java/com/box/l10n/mojito/entity/ScheduledJobTypeEntity.java
Outdated
Show resolved
Hide resolved
webapp/src/main/java/com/box/l10n/mojito/rest/scheduledjob/ScheduledJobWS.java
Outdated
Show resolved
Hide resolved
…vel, Made endpoints require POST request
…cidents/resolves that fail.
mattwilshire
force-pushed
the
mattwilshire/v1-thirdparty-sync-migration
branch
from
0b9f4fa
to
a9a1336
Compare
| @Override | ||
| public boolean vetoJobExecution(Trigger trigger, JobExecutionContext context) { | ||
| // If the job is disabled, don't execute | ||
| ScheduledJob job = scheduledJobRepository.findByJobKey(trigger.getJobKey()); |
There was a problem hiding this comment.
Should we perhaps handle the case where the job might not be found?
There was a problem hiding this comment.
Unlikely, I know but not impossible
There was a problem hiding this comment.
Out of date comment @byronantak
webapp/src/main/java/com/box/l10n/mojito/service/scheduledjob/ScheduledJobTypeRepository.java
Outdated
Show resolved
Hide resolved
| package com.box.l10n.mojito.service.scheduledjob; | ||
|
|
||
| public abstract class ScheduledJobProperties { | ||
| private int version = 1; |
There was a problem hiding this comment.
I'm wondering if we should use semantic versioning here 🤔 Probably overkill
There was a problem hiding this comment.
No, this is only for casting to newer versions, it doesn't require semantic versioning.
webapp/src/main/java/com/box/l10n/mojito/service/scheduledjob/ScheduledJobRepository.java
Outdated
Show resolved
Hide resolved
webapp/src/main/java/com/box/l10n/mojito/entity/ScheduledJob.java
Outdated
Show resolved
Hide resolved
| @Column(name = "name") | ||
| @Enumerated(EnumType.STRING) | ||
| @JsonView(View.Repository.class) | ||
| private com.box.l10n.mojito.service.scheduledjob.ScheduledJobStatus jobStatus; |
There was a problem hiding this comment.
Can we use an import here? This looks funky to me
There was a problem hiding this comment.
No we can't, the name of the imported class matches the name of the current class, the definition needs to be this specific.
| @Column(name = "name") | ||
| @Enumerated(EnumType.STRING) | ||
| @JsonView(View.Repository.class) | ||
| private com.box.l10n.mojito.service.scheduledjob.ScheduledJobType jobType; |
There was a problem hiding this comment.
Similar comment as before
There was a problem hiding this comment.
Check previous comment.
| public class ScheduledJobType extends BaseEntity { | ||
| @Id private Long id; | ||
|
|
||
| @Basic(optional = false) |
There was a problem hiding this comment.
Hold on. Why is this needed? The column is not not nullable, so should the config not reflect that. This Basic property to my understanding enforces that the property is not null locally but not at a db level (I might be wrong)
There was a problem hiding this comment.
I have updated the other columns that are not nullable to have this, I noticed it was on other entity objects so I decided to use it. It looks like you can generate SQL schemas off of entity mappings, not that we would need to use this but best practice to have annotations like this!
| new ResponseStatusException(HttpStatus.NOT_FOUND, "Job not found with id: " + id)); | ||
| } | ||
|
|
||
| @RequestMapping(method = RequestMethod.POST, value = "/api/jobs/{id}") |
There was a problem hiding this comment.
Just looking at a POST to "/api/jobs/{id}" suggests to me that we might be updating some information for that job or submitting a new/updated job definition (as the get at on the same path/route returns us back the information about the job).
(We might also want to reserve the POST/PUT to that route for dynamically creating job definitions in the future)
Thinking of using something more explicit here, similar to enable/disable, e.g.:
"/api/jobs/{id}/trigger" ?
| @Audited | ||
| @Entity | ||
| @Table(name = "scheduled_job") | ||
| public class ScheduledJob { |
There was a problem hiding this comment.
Should we extend BaseEntity here, which will then provide the id field handling in the parent class? Might have missed a previous conversation about this
| JobKey jobKey = scheduledJobManager.getJobKey(scheduledJob); | ||
|
|
||
| try { | ||
| if (!scheduledJobManager.getScheduler().checkExists(jobKey)) return notFoundResponse; |
There was a problem hiding this comment.
Can getScheduler() return null in any case? If so do we need handling for that here?
| this.endDate = endDate; | ||
| } | ||
|
|
||
| public Boolean getEnabled() { |
There was a problem hiding this comment.
Should this be named isEnabled()?
