Skip to content
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

v1 Scheduled Jobs / Third Party Sync Migration #171

Draft
wants to merge 34 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
0d314b6
Added a scheduled job manager to handle the creation of cron jobs loc…
mattwilshire Sep 25, 2024
7aaed92
Added main logic, storing jobs to database and parsing them on retrie…
mattwilshire Oct 1, 2024
9edb9fe
Added ScheduledJobDTO for API endpoints and handled a possible row in…
mattwilshire Oct 1, 2024
fd73ff5
Fix failing maven tests.
mattwilshire Oct 1, 2024
3fb087b
Added SQL migration. Removed isAllowedExecutionOverlap, as the Disall…
mattwilshire Oct 2, 2024
2b17972
Changed conditional property to match current quartz condition.
mattwilshire Oct 2, 2024
d1cba21
Attached the job properties to the Job Type in the enum definition
mattwilshire Oct 2, 2024
3e49061
Added Slack Notification for failed third party sync
mattwilshire Oct 2, 2024
6a6d61c
Fix failing maven tests
mattwilshire Oct 2, 2024
2c2d80c
Trigger mvn tests again
mattwilshire Oct 2, 2024
6bbe94d
Fix maven tests ?
mattwilshire Oct 2, 2024
ae0fff4
Fix exception thrown if @Value is missing
mattwilshire Oct 2, 2024
8dea68d
Changed condition on property value
mattwilshire Oct 2, 2024
427b4f0
Send pollable task ID in notification and other small changes.
mattwilshire Oct 4, 2024
14ba12b
Denormalized job type and job status. Added version to base scheduled…
mattwilshire Oct 11, 2024
4aeaccf
Cleanup & Set end / start date to null depending on current job status
mattwilshire Oct 16, 2024
85f141d
Added more ScheduledJob endpoints and veto the job execution if its d…
mattwilshire Oct 17, 2024
30dac28
Added ScheduledJobResponse for structured API responses
mattwilshire Oct 17, 2024
9a42a6b
Use UUID for job id
mattwilshire Oct 17, 2024
4ce23ff
Fix failing tests
mattwilshire Oct 18, 2024
3efecaf
Fix deadlock that would occur in audit table when two syncs started a…
mattwilshire Oct 21, 2024
246597e
Clean up map
mattwilshire Oct 21, 2024
348a15d
Refactoring & Added PagerDuty incident creation on job failure
mattwilshire Oct 22, 2024
bcaef3f
Add flyway schema & Add PD incident title to configuration
mattwilshire Oct 22, 2024
f44b540
Added configuration examples, Fix deadlock occurring at the method le…
mattwilshire Oct 22, 2024
40ddd6f
Comment changes
mattwilshire Oct 23, 2024
7f41168
Add starting tests, ensuring they work in HSQL
mattwilshire Oct 23, 2024
94208dd
Tests check
mattwilshire Oct 23, 2024
fff8a9f
Fix tests by not using new application context
mattwilshire Oct 25, 2024
fc8cdaf
Fix tests by manually adding rows flyway would handle that HSQL doesn…
mattwilshire Oct 25, 2024
682c2bc
Review comments to fix up SQL schema
mattwilshire Oct 25, 2024
60277d6
Review comments, ZonedDateTime & Foreign Key, BIT changes
mattwilshire Oct 25, 2024
09135bc
Capture PathVariable using UUID class
mattwilshire Oct 25, 2024
d97ceb3
Review comment changes and refactoring
mattwilshire Oct 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
153 changes: 153 additions & 0 deletions webapp/src/main/java/com/box/l10n/mojito/entity/ScheduledJob.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
package com.box.l10n.mojito.entity;

import com.box.l10n.mojito.json.ObjectMapper;
import com.box.l10n.mojito.service.scheduledjob.ScheduledJobProperties;
import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.persistence.Id;
import jakarta.persistence.JoinColumn;
import jakarta.persistence.ManyToOne;
import jakarta.persistence.PostLoad;
import jakarta.persistence.Table;
import jakarta.persistence.Transient;
import java.util.Date;
import org.hibernate.envers.Audited;

@Audited
@Entity
@Table(name = "scheduled_job")
public class ScheduledJob {
@Id private String id;

@ManyToOne
@JoinColumn(name = "repository_id")
private Repository repository;
mattwilshire marked this conversation as resolved.
Show resolved Hide resolved

@ManyToOne
@JoinColumn(name = "job_type")
private ScheduledJobTypeEntity jobType;
mattwilshire marked this conversation as resolved.
Show resolved Hide resolved

@Column(name = "cron")
private String cron;

@Transient private ScheduledJobProperties properties;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@mattwilshire mattwilshire Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


@Column(name = "properties")
private String propertiesString;

@ManyToOne
@JoinColumn(name = "job_status")
private ScheduledJobStatusEntity jobStatus;

@Column(name = "start_date")
private Date startDate;
mattwilshire marked this conversation as resolved.
Show resolved Hide resolved

@Column(name = "end_date")
private Date endDate;
mattwilshire marked this conversation as resolved.
Show resolved Hide resolved

@Column(name = "enabled", nullable = false, columnDefinition = "TINYINT DEFAULT 1")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

private Boolean enabled = true;

@PostLoad
public void deserializeProperties() {
ObjectMapper objectMapper = new ObjectMapper();
try {
this.properties =
objectMapper.readValue(propertiesString, jobType.getEnum().getPropertiesClass());
} catch (Exception e) {
throw new RuntimeException(
"Failed to deserialize properties '"
+ propertiesString
+ "' for class: "
+ jobType.getEnum().getPropertiesClass().getName(),
e);
}
}

public String getId() {
return id;
}

public void setId(String id) {
this.id = id;
}

public Repository getRepository() {
return repository;
}

public void setRepository(Repository repository) {
this.repository = repository;
}

public ScheduledJobTypeEntity getJobType() {
return jobType;
}

public void setJobType(ScheduledJobTypeEntity jobType) {
this.jobType = jobType;
}

public String getCron() {
return cron;
}

public void setCron(String cron) {
this.cron = cron;
}

public ScheduledJobProperties getProperties() {
return properties;
}

public void setProperties(ScheduledJobProperties properties) {
this.properties = properties;

ObjectMapper objectMapper = new ObjectMapper();
try {
this.propertiesString = objectMapper.writeValueAsString(this.properties);
} catch (Exception e) {
throw new RuntimeException("Failed to serialize properties", e);
}
}

public String getPropertiesString() {
return propertiesString;
}

public void setPropertiesString(String properties) {
this.propertiesString = properties;
}

public ScheduledJobStatusEntity getJobStatus() {
return jobStatus;
}

public void setJobStatus(ScheduledJobStatusEntity jobStatus) {
this.jobStatus = jobStatus;
}

public Date getStartDate() {
return startDate;
}

public void setStartDate(Date startDate) {
this.startDate = startDate;
}

public Date getEndDate() {
return endDate;
}

public void setEndDate(Date endDate) {
this.endDate = endDate;
}

public Boolean getEnabled() {
return enabled;
}

public void setEnabled(Boolean enabled) {
this.enabled = enabled;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package com.box.l10n.mojito.entity;

import static org.hibernate.envers.RelationTargetAuditMode.NOT_AUDITED;

import com.box.l10n.mojito.rest.View;
import com.box.l10n.mojito.service.scheduledjob.ScheduledJobStatus;
import com.fasterxml.jackson.annotation.JsonView;
import jakarta.persistence.Basic;
import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.persistence.EnumType;
import jakarta.persistence.Enumerated;
import jakarta.persistence.Id;
import jakarta.persistence.Table;
import org.hibernate.envers.Audited;

@Entity
@Table(name = "scheduled_job_status_type")
@Audited(targetAuditMode = NOT_AUDITED)
mattwilshire marked this conversation as resolved.
Show resolved Hide resolved
public class ScheduledJobStatusEntity extends BaseEntity {
@Id private Long id;

@Basic(optional = false)
@Column(name = "name")
@Enumerated(EnumType.STRING)
@JsonView(View.Repository.class)
private ScheduledJobStatus jobStatus;

public ScheduledJobStatus getEnum() {
return jobStatus;
}

public void setJobStatus(ScheduledJobStatus jobStatus) {
this.jobStatus = jobStatus;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package com.box.l10n.mojito.entity;

import static org.hibernate.envers.RelationTargetAuditMode.NOT_AUDITED;

import com.box.l10n.mojito.rest.View;
import com.box.l10n.mojito.service.scheduledjob.ScheduledJobType;
import com.fasterxml.jackson.annotation.JsonView;
import jakarta.persistence.Basic;
import jakarta.persistence.Column;
import jakarta.persistence.Entity;
import jakarta.persistence.EnumType;
import jakarta.persistence.Enumerated;
import jakarta.persistence.Id;
import jakarta.persistence.Table;
import org.hibernate.envers.Audited;

@Entity
@Table(name = "scheduled_job_type")
@Audited(targetAuditMode = NOT_AUDITED)
public class ScheduledJobTypeEntity extends BaseEntity {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Id private Long id;

@Basic(optional = false)
@Column(name = "name")
@Enumerated(EnumType.STRING)
@JsonView(View.Repository.class)
private ScheduledJobType jobType;

@Override
public Long getId() {
return id;
}

@Override
public void setId(Long id) {
this.id = id;
}

public ScheduledJobType getEnum() {
return jobType;
}

public void setEnum(ScheduledJobType jobType) {
this.jobType = jobType;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
package com.box.l10n.mojito.rest.scheduledjob;

import com.box.l10n.mojito.entity.ScheduledJob;
import com.box.l10n.mojito.service.scheduledjob.ScheduledJobDTO;
import com.box.l10n.mojito.service.scheduledjob.ScheduledJobManager;
import com.box.l10n.mojito.service.scheduledjob.ScheduledJobRepository;
import com.box.l10n.mojito.service.scheduledjob.ScheduledJobResponse;
import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;
import org.quartz.JobExecutionContext;
import org.quartz.JobKey;
import org.quartz.SchedulerException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestMethod;
import org.springframework.web.bind.annotation.ResponseStatus;
import org.springframework.web.bind.annotation.RestController;
import org.springframework.web.server.ResponseStatusException;

/**
* Webservice for enabling/disabling and triggering scheduled jobs.
*
* @author mattwilshire
*/
@RestController
@ConditionalOnProperty(value = "l10n.scheduledJobs.enabled", havingValue = "true")
public class ScheduledJobWS {
mattwilshire marked this conversation as resolved.
Show resolved Hide resolved

static Logger logger = LoggerFactory.getLogger(ScheduledJobWS.class);

@Autowired ScheduledJobRepository scheduledJobRepository;
@Autowired ScheduledJobManager scheduledJobManager;

@RequestMapping(method = RequestMethod.GET, value = "/api/jobs")
@ResponseStatus(HttpStatus.OK)
public List<ScheduledJobDTO> getJobs() {
List<ScheduledJob> scheduledJobs = scheduledJobRepository.findAll();
return scheduledJobs.stream().map(ScheduledJobDTO::new).collect(Collectors.toList());
}

@RequestMapping(method = RequestMethod.GET, value = "/api/jobs/{id}")
@ResponseStatus(HttpStatus.OK)
public ScheduledJobDTO getJob(@PathVariable String id) {
return scheduledJobRepository
.findById(id)
.map(ScheduledJobDTO::new)
.orElseThrow(
() ->
new ResponseStatusException(HttpStatus.NOT_FOUND, "Job not found with id: " + id));
}

@RequestMapping(method = RequestMethod.POST, value = "/api/jobs/{id}")
@ResponseStatus(HttpStatus.OK)
public ResponseEntity<ScheduledJobResponse> triggerJob(@PathVariable String id) {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Optional<ScheduledJob> optionalScheduledJob = scheduledJobRepository.findById(id);
if (optionalScheduledJob.isEmpty())
return ResponseEntity.status(HttpStatus.NOT_FOUND)
.body(new ScheduledJobResponse(ScheduledJobResponse.Status.ERROR, "Job not found"));

ScheduledJob scheduledJob = optionalScheduledJob.get();
JobKey jobKey = scheduledJobManager.getJobKey(scheduledJob);

try {
if (!scheduledJobManager.getScheduler().checkExists(jobKey))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

return ResponseEntity.status(HttpStatus.NOT_FOUND)
.body(new ScheduledJobResponse(ScheduledJobResponse.Status.ERROR, "Job doesn't exist"));

// Is the job currently running ?
// Ignore the trigger request
for (JobExecutionContext jobExecutionContext :
scheduledJobManager.getScheduler().getCurrentlyExecutingJobs()) {
if (jobExecutionContext.getJobDetail().getKey().equals(jobKey)) {
return ResponseEntity.status(HttpStatus.CONFLICT)
.body(
new ScheduledJobResponse(
ScheduledJobResponse.Status.ERROR,
"Job is currently running, trigger request ignored"));
}
}

scheduledJobManager.getScheduler().triggerJob(jobKey);
return ResponseEntity.status(HttpStatus.OK)
.body(new ScheduledJobResponse(ScheduledJobResponse.Status.SUCCESS, "Job triggered"));
} catch (SchedulerException e) {
logger.error(
"Error triggering job manually, job: {}", jobKey.getName() + ":" + jobKey.getGroup(), e);
return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR)
.body(new ScheduledJobResponse(ScheduledJobResponse.Status.ERROR, e.getMessage()));
}
}

@RequestMapping(method = RequestMethod.POST, value = "/api/jobs/{id}/toggle")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ResponseStatus(HttpStatus.OK)
public ResponseEntity<ScheduledJobResponse> toggleJob(@PathVariable String id) {
Optional<ScheduledJob> optionalScheduledJob = scheduledJobRepository.findById(id);
if (optionalScheduledJob.isEmpty())
return ResponseEntity.status(HttpStatus.NOT_FOUND)
.body(new ScheduledJobResponse(ScheduledJobResponse.Status.ERROR, "Job not found"));

ScheduledJob scheduledJob = optionalScheduledJob.get();
JobKey jobKey = scheduledJobManager.getJobKey(scheduledJob);

try {
if (!scheduledJobManager.getScheduler().checkExists(jobKey))
return ResponseEntity.status(HttpStatus.NOT_FOUND)
.body(new ScheduledJobResponse(ScheduledJobResponse.Status.ERROR, "Job doesn't exist"));

scheduledJob.setEnabled(!scheduledJob.getEnabled());
scheduledJobRepository.save(scheduledJob);

return ResponseEntity.status(HttpStatus.OK)
.body(
new ScheduledJobResponse(
ScheduledJobResponse.Status.SUCCESS,
"Job " + (scheduledJob.getEnabled() ? "enabled" : "disabled")));
} catch (SchedulerException e) {
throw new ResponseStatusException(
HttpStatus.INTERNAL_SERVER_ERROR, "Job with id: " + id + " could not be disabled");
}
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.box.l10n.mojito.retry;

import com.google.common.collect.ImmutableMap;
import org.springframework.dao.CannotAcquireLockException;
import org.springframework.dao.DeadlockLoserDataAccessException;
import org.springframework.retry.RetryCallback;
import org.springframework.retry.backoff.ExponentialRandomBackOffPolicy;
Expand All @@ -15,7 +16,13 @@ public class DeadLockLoserExceptionRetryTemplate {

public DeadLockLoserExceptionRetryTemplate() {
SimpleRetryPolicy retryPolicy =
new SimpleRetryPolicy(5, ImmutableMap.of(DeadlockLoserDataAccessException.class, true));
new SimpleRetryPolicy(
5,
ImmutableMap.of(
DeadlockLoserDataAccessException.class,
true,
CannotAcquireLockException.class,
true));

ExponentialRandomBackOffPolicy exponentialRandomBackOffPolicy =
new ExponentialRandomBackOffPolicy();
Expand Down
Loading