v1 Scheduled Jobs / Third Party Sync Migration

[mattwilshire]

• 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.

[byronantak]

I don't see this column definition used in the repo before. Is it necessary?

[byronantak]

Also is the nullable false necessary since you default the field value to false?

[garionpin]

Ideally, we would use the "BIT" column type here - as that is the closest semantical equivalent for storing a bool in MySQL and is the smallest amount of space used.

https://dev.mysql.com/doc/refman/8.0/en/bit-type.html

[byronantak]

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?

[byronantak]

Nit: toggle is a strange term for this. I get that you mean it disable and enable a job, but this name feels confusing

[byronantak]

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

[mattwilshire]

Sounds good, I'll change it to two separate endpoints!

[byronantak]

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...

[mattwilshire]

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.

[byronantak]

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.

[mattwilshire]

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.

[byronantak]

Should this not be the try as well?
What if the cast fails or if the key is not found (by some misfortune)

[mattwilshire]

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.

[byronantak]

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)

[garionpin]

Out of curiosity - is the use of @transient here to do with some desired serialization behaviour?

[mattwilshire]

Yes that's exactly what its for in this instance, the transient here indicates to the JPA that the field should not be persisted to the database and when the row is pulled from the db, the deserializeProperties method in the entity is called as it has the PostLoad annotation. The properties string from the db is deserialized into this transient field. The setter of this transient field converts itself back into a JSON string which is stored in the propertiesString field that is persisted to the db.

[garionpin]

Small note to track that changes to the base table column types also get reflected the _aud version

[garionpin]

For the actual entity class names - we don't tend to suffix them with the word "Entity", e.g.:
public class Locale extends BaseEntity (vs. LocaleEntity)

[mattwilshire]

I ended up using Entity for the suffix here as I have another class labelled ScheduledJobType in another package that I created (You will see it further on in the PR). Would the best approach be to rename that ScheduledJobType to something else so that the entity version can use it ? I'm thinking of calling the current one : ScheduledJobTypeEnum as the enum currently uses that name.

[garionpin]

The id here should probably be of type long instead of string

[mattwilshire]

The id here is a UUID. However, I have changed the data type here to be a UUID so that Spring will respond with a 400 Bad Request if it isn't a valid UUID passed through.