Skip to content

Commit

Permalink
SAK-32349 Fix peer assessment and ScheduledInvocationManager (sakaipr…
Browse files Browse the repository at this point in the history
…oject#4114)

* SAK-32349 Cope with missing trigger.

This fixes the issue.

* SAK-32349 Add tests that attempt to check bug.

This attempts to check that when a trigger doesn’t exist for a scheduled job things work as best they can.

* SAK-32349 Use a new transaction for changes.

This is a good solution because the calling code (in assignments) had it’s own hibernate transaction already established. The quartz code commits it’s transaction when it returns and so then second call through to quartz wouldn’t find the trigger that had just been deleted.

So the fix to have the ScheduledInvocationManager use a new transaction for each call so it remains in sync with the quartz datastore.

* SAK-32349 Update to use lombok on ContextMapping.
  • Loading branch information
buckett authored and ern committed Mar 21, 2017
1 parent 4489a5c commit ba17615
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 78 deletions.
Original file line number Diff line number Diff line change
@@ -1,24 +1,37 @@
package org.sakaiproject.component.app.scheduler;

import java.time.Instant;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Date;
import java.util.List;

import org.hibernate.NonUniqueObjectException;
import org.quartz.*;
import org.quartz.JobBuilder;
import org.quartz.JobDetail;
import org.quartz.JobExecutionContext;
import org.quartz.JobKey;
import org.quartz.ListenerManager;
import org.quartz.ObjectAlreadyExistsException;
import org.quartz.Scheduler;
import org.quartz.SchedulerException;
import org.quartz.SchedulerFactory;
import org.quartz.Trigger;
import org.quartz.TriggerBuilder;
import org.quartz.TriggerKey;
import org.quartz.TriggerListener;
import org.quartz.impl.matchers.GroupMatcher;
import org.quartz.listeners.TriggerListenerSupport;
import org.sakaiproject.component.app.scheduler.jobs.ScheduledInvocationJob;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.sakaiproject.api.app.scheduler.DelayedInvocation;
import org.sakaiproject.api.app.scheduler.ScheduledInvocationManager;
import org.sakaiproject.component.app.scheduler.jobs.ScheduledInvocationJob;
import org.sakaiproject.id.api.IdManager;
import org.sakaiproject.time.api.Time;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.transaction.annotation.Transactional;

import java.time.Instant;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Date;
import java.util.List;

import static org.springframework.transaction.annotation.Propagation.REQUIRES_NEW;

/**
* componentId -> job key (name)
* opaqueContent/contextId -> trigger key (name)
Expand Down Expand Up @@ -76,14 +89,14 @@ public void destroy() throws SchedulerException {
}

@Override
@Transactional
@Transactional(propagation = REQUIRES_NEW)
public String createDelayedInvocation(Time time, String componentId, String opaqueContext) {
Instant instant = Instant.ofEpochMilli(time.getTime());
return createDelayedInvocation(instant, componentId, opaqueContext);
}

@Override
@Transactional
@Transactional(propagation = REQUIRES_NEW)
public String createDelayedInvocation(Instant instant, String componentId, String opaqueContext) {
String uuid = m_idManager.createUuid();
createDelayedInvocation(instant, componentId, opaqueContext, uuid);
Expand All @@ -95,15 +108,14 @@ public String createDelayedInvocation(Instant instant, String componentId, Strin
* and specify the UUID that should be used.
* @see org.sakaiproject.component.app.scheduler.jobs.SchedulerMigrationJob
*/
@Transactional
@Transactional(propagation = REQUIRES_NEW)
public void createDelayedInvocation(Instant instant, String componentId, String opaqueContext, String uuid) {
try {
dao.add(uuid, componentId, opaqueContext);
} catch (NonUniqueObjectException nuoe) {
// Delete the existing one.
String oldUuid = dao.get(componentId, opaqueContext);
// Delete the existing one.
if (oldUuid != null) {
deleteDelayedInvocation(componentId, opaqueContext);
dao.add(uuid, componentId, opaqueContext);
}
dao.add(uuid, componentId, opaqueContext);
try {
Scheduler scheduler = schedulerFactory.getScheduler();
JobKey key = new JobKey(componentId, GROUP_NAME);
Expand Down Expand Up @@ -141,7 +153,7 @@ public void createDelayedInvocation(Instant instant, String componentId, String
/* (non-Javadoc)
* @see org.sakaiproject.api.app.scheduler.ScheduledInvocationManager#deleteDelayedInvocation(java.lang.String)
*/
@Transactional
@Transactional(propagation = REQUIRES_NEW)
public void deleteDelayedInvocation(String uuid) {

LOG.debug("Removing Delayed Invocation: " + uuid);
Expand All @@ -158,22 +170,20 @@ public void deleteDelayedInvocation(String uuid) {
/* (non-Javadoc)
* @see org.sakaiproject.api.app.scheduler.ScheduledInvocationManager#deleteDelayedInvocation(java.lang.String, java.lang.String)
*/
@Transactional
@Transactional(propagation = REQUIRES_NEW)
public void deleteDelayedInvocation(String componentId, String opaqueContext) {
LOG.debug("componentId=" + componentId + ", opaqueContext=" + opaqueContext);

DelayedInvocation[] delayedInvocations = findDelayedInvocations(componentId, opaqueContext);
if (delayedInvocations != null) {
for (DelayedInvocation delayedInvocation : delayedInvocations) {
deleteDelayedInvocation(delayedInvocation.uuid);
}
Collection<String> uuids = dao.find(componentId, opaqueContext);
for (String uuid: uuids) {
deleteDelayedInvocation(uuid);
}
}

/* (non-Javadoc)
* @see org.sakaiproject.api.app.scheduler.ScheduledInvocationManager#findDelayedInvocations(java.lang.String, java.lang.String)
*/
@Transactional
@Transactional(propagation = REQUIRES_NEW)
public DelayedInvocation[] findDelayedInvocations(String componentId, String opaqueContext) {
LOG.debug("componentId=" + componentId + ", opaqueContext=" + opaqueContext);
Collection<String> uuids = dao.find(componentId, opaqueContext);
Expand All @@ -182,8 +192,11 @@ public DelayedInvocation[] findDelayedInvocations(String componentId, String opa
TriggerKey key = new TriggerKey(uuid, GROUP_NAME);
try {
Trigger trigger = schedulerFactory.getScheduler().getTrigger(key);
invocations.add(new DelayedInvocation(trigger.getKey().getName(), trigger.getNextFireTime(), key.getName(), opaqueContext));

if (trigger == null) {
LOG.error("Failed to trigger with key: {}", key);
} else {
invocations.add(new DelayedInvocation(trigger.getKey().getName(), trigger.getNextFireTime(), key.getName(), opaqueContext));
}
} catch (SchedulerException e) {
LOG.warn("Problem finding delayed invocations.", e);
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,4 +105,23 @@ public void testCreateMultipleContexts() {
Assert.assertNotEquals(first, second);
}

@Test
public void testFindMissingTrigger() {
// Check that find still work when a trigger can't be found for a delayed invocation
// Here we have an entry in the dao, but there won't be a trigger.
Mockito.when(dao.find(COMPONENT_ID, CONTEXT)).thenReturn(Collections.singleton("uuid"));
DelayedInvocation[] delayedInvocations = manager.findDelayedInvocations(COMPONENT_ID, CONTEXT);
Assert.assertTrue(delayedInvocations.length == 0);
}

@Test
public void testCreateMissingTrigger() {
// Check that creating a delayed invokation still works.
// Here we have an entry in the dao, but there won't be a trigger.
Mockito.when(dao.find(COMPONENT_ID, CONTEXT)).thenReturn(Collections.singleton("uuid"));
Mockito.when(dao.get(COMPONENT_ID, CONTEXT)).thenReturn("uuid");
String uuid = manager.createDelayedInvocation(Instant.now(), COMPONENT_ID, CONTEXT);
Assert.assertNotNull(uuid);
}

}
Original file line number Diff line number Diff line change
@@ -1,67 +1,42 @@
package org.sakaiproject.scheduler.events.hibernate;

import javax.persistence.*;
import lombok.EqualsAndHashCode;
import lombok.Getter;
import lombok.Setter;
import lombok.ToString;

import javax.persistence.Entity;
import javax.persistence.Id;
import javax.persistence.Table;
import javax.persistence.UniqueConstraint;

/**
* Just maps a context ID, component ID to a quartz trigger UUID.
*/
@Entity(name = "context_mapping")
@Table(uniqueConstraints={@UniqueConstraint(columnNames = {"componentId", "contextId"})})
@EqualsAndHashCode(exclude = {"uuid"})
@ToString
public class ContextMapping {

/**
*This is the ID of the quartz trigger
*/
@Id
// This is the ID of the quartz trigger
@Getter @Setter
private String uuid;

// This is the context ID (opaque ID) passed when the job was created.
/**
* This is the context ID (opaque ID) passed when the job was created.
*/
@Getter @Setter
private String contextId;

// This is the component ID, we have this as a separate column so that overlapping context IDs (opaque IDs) aren't
// a problem.
/**
* This is the component ID, we have this as a separate column so that overlapping context IDs (opaque IDs) aren't
* a problem.
*/
@Getter @Setter
private String componentId;

public String getContextId() {
return contextId;
}

public void setContextId(String contextId) {
this.contextId = contextId;
}

public String getUuid() {
return uuid;
}

public void setUuid(String uuid) {
this.uuid = uuid;
}

public String getComponentId() {
return componentId;
}

public void setComponentId(String componentId) {
this.componentId = componentId;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;

ContextMapping that = (ContextMapping) o;

if (!uuid.equals(that.uuid)) return false;
if (!contextId.equals(that.contextId)) return false;
return componentId.equals(that.componentId);

}

@Override
public int hashCode() {
int result = uuid.hashCode();
result = 31 * result + contextId.hashCode();
result = 31 * result + componentId.hashCode();
return result;
}
}

0 comments on commit ba17615

Please sign in to comment.