From 5cafea2bde5d7ad06e65f7c984ad33a15987cd0d Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Wed, 9 Oct 2024 12:22:10 -0400 Subject: [PATCH] Add the concept of queuePriority to jobs. --- .../securesms/database/JobDatabase.kt | 27 ++- .../securesms/jobmanager/Job.java | 62 ++++-- .../securesms/jobmanager/JobController.java | 6 +- .../securesms/jobmanager/JobMigrator.java | 3 +- .../jobmanager/persistence/JobSpec.kt | 5 +- .../jobs/ConversationShortcutUpdateJob.java | 2 +- .../securesms/jobs/FastJobStorage.kt | 53 ++--- .../securesms/jobs/MinimalJobSpec.kt | 3 +- .../jobs/PushGroupSilentUpdateSendJob.java | 2 +- .../jobs/RestoreAttachmentThumbnailJob.kt | 2 +- .../securesms/jobmanager/JobMigratorTest.java | 2 +- .../securesms/jobs/FastJobStorageTest.kt | 195 +++++++++++++++--- 12 files changed, 274 insertions(+), 88 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/JobDatabase.kt b/app/src/main/java/org/thoughtcrime/securesms/database/JobDatabase.kt index 42d35c8530..b5f5a9efed 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/JobDatabase.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/JobDatabase.kt @@ -63,7 +63,8 @@ class JobDatabase( const val SERIALIZED_DATA = "serialized_data" const val SERIALIZED_INPUT_DATA = "serialized_input_data" const val IS_RUNNING = "is_running" - const val PRIORITY = "priority" + const val GLOBAL_PRIORITY = "global_priority" + const val QUEUE_PRIORITY = "queue_priority" val CREATE_TABLE = """ @@ -81,7 +82,8 @@ class JobDatabase( $SERIALIZED_INPUT_DATA TEXT DEFAULT NULL, $IS_RUNNING INTEGER, $NEXT_BACKOFF_INTERVAL INTEGER, - $PRIORITY INTEGER DEFAULT 0 + $GLOBAL_PRIORITY INTEGER DEFAULT 0, + $QUEUE_PRIORITY INTEGER DEFAULT 0 ) """.trimIndent() } @@ -140,6 +142,11 @@ class JobDatabase( if (oldVersion < 3) { db.execSQL("ALTER TABLE job_spec ADD COLUMN priority INTEGER DEFAULT 0") } + + if (oldVersion < 4) { + db.execSQL("ALTER TABLE job_spec RENAME COLUMN priority TO global_priority") + db.execSQL("ALTER TABLE job_spec ADD COLUMN queue_priority INTEGER DEFAULT 0") + } } override fun onOpen(db: SQLiteDatabase) { @@ -179,7 +186,7 @@ class JobDatabase( .select() .from(Jobs.TABLE_NAME) .where("${Jobs.QUEUE_KEY} = ?", queue) - .orderBy("${Jobs.PRIORITY} DESC, ${Jobs.CREATE_TIME} ASC, ${Jobs.ID} ASC") + .orderBy("${Jobs.GLOBAL_PRIORITY} DESC, ${Jobs.QUEUE_PRIORITY} DESC, ${Jobs.CREATE_TIME} ASC, ${Jobs.ID} ASC") .limit(1) .run() .readToSingleObject { it.toJobSpec() } @@ -224,7 +231,8 @@ class JobDatabase( Jobs.LAST_RUN_ATTEMPT_TIME, Jobs.NEXT_BACKOFF_INTERVAL, Jobs.IS_RUNNING, - Jobs.PRIORITY + Jobs.GLOBAL_PRIORITY, + Jobs.QUEUE_PRIORITY ) return readableDatabase .query(Jobs.TABLE_NAME, columns, null, null, null, null, "${Jobs.CREATE_TIME}, ${Jobs.ID} ASC") @@ -236,7 +244,8 @@ class JobDatabase( createTime = cursor.requireLong(Jobs.CREATE_TIME), lastRunAttemptTime = cursor.requireLong(Jobs.LAST_RUN_ATTEMPT_TIME), nextBackoffInterval = cursor.requireLong(Jobs.NEXT_BACKOFF_INTERVAL), - priority = cursor.requireInt(Jobs.PRIORITY), + globalPriority = cursor.requireInt(Jobs.GLOBAL_PRIORITY), + queuePriority = cursor.requireInt(Jobs.QUEUE_PRIORITY), isRunning = cursor.requireBoolean(Jobs.IS_RUNNING), isMemoryOnly = false ) @@ -440,7 +449,8 @@ class JobDatabase( serializedInputData = this.requireBlob(Jobs.SERIALIZED_INPUT_DATA), isRunning = this.requireBoolean(Jobs.IS_RUNNING), isMemoryOnly = false, - priority = this.requireInt(Jobs.PRIORITY) + globalPriority = this.requireInt(Jobs.GLOBAL_PRIORITY), + queuePriority = this.requireInt(Jobs.QUEUE_PRIORITY) ) } @@ -483,13 +493,14 @@ class JobDatabase( Jobs.SERIALIZED_DATA to this.serializedData, Jobs.SERIALIZED_INPUT_DATA to this.serializedInputData, Jobs.IS_RUNNING to if (this.isRunning) 1 else 0, - Jobs.PRIORITY to this.priority + Jobs.GLOBAL_PRIORITY to this.globalPriority, + Jobs.QUEUE_PRIORITY to this.queuePriority ) } companion object { private val TAG = Log.tag(JobDatabase::class.java) - private const val DATABASE_VERSION = 3 + private const val DATABASE_VERSION = 4 private const val DATABASE_NAME = "signal-jobmanager.db" @SuppressLint("StaticFieldLeak") diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobmanager/Job.java b/app/src/main/java/org/thoughtcrime/securesms/jobmanager/Job.java index b4dfea8e12..23b0b7afb0 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobmanager/Job.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobmanager/Job.java @@ -2,6 +2,7 @@ package org.thoughtcrime.securesms.jobmanager; import android.content.Context; +import androidx.annotation.IntDef; import androidx.annotation.NonNull; import androidx.annotation.Nullable; import androidx.annotation.WorkerThread; @@ -10,12 +11,15 @@ import org.signal.core.util.logging.Log; import org.thoughtcrime.securesms.jobmanager.impl.BackoffUtil; import org.thoughtcrime.securesms.util.RemoteConfig; +import java.lang.annotation.Retention; import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.UUID; +import static java.lang.annotation.RetentionPolicy.SOURCE; + /** * A durable unit of work. * @@ -263,6 +267,9 @@ public abstract class Job { public static final long IMMORTAL = -1; public static final int UNLIMITED = -1; + @Retention(SOURCE) + @IntDef({ PRIORITY_DEFAULT, PRIORITY_LOW, PRIORITY_HIGH}) + public @interface Priority{} public static final int PRIORITY_DEFAULT = 0; public static final int PRIORITY_HIGH = 1; public static final int PRIORITY_LOW = -1; @@ -277,7 +284,8 @@ public abstract class Job { private final List constraintKeys; private final byte[] inputData; private final boolean memoryOnly; - private final int priority; + private final int globalPriority; + private final int queuePriority; private Parameters(@NonNull String id, long createTime, @@ -289,7 +297,8 @@ public abstract class Job { @NonNull List constraintKeys, @Nullable byte[] inputData, boolean memoryOnly, - int priority) + int globalPriority, + int queuePriority) { this.id = id; this.createTime = createTime; @@ -301,7 +310,8 @@ public abstract class Job { this.constraintKeys = constraintKeys; this.inputData = inputData; this.memoryOnly = memoryOnly; - this.priority = priority; + this.globalPriority = globalPriority; + this.queuePriority = queuePriority; } @NonNull String getId() { @@ -344,12 +354,16 @@ public abstract class Job { return memoryOnly; } - int getPriority() { - return priority; + int getGlobalPriority() { + return globalPriority; + } + + int getQueuePriority() { + return queuePriority; } public Builder toBuilder() { - return new Builder(id, createTime, lifespan, maxAttempts, maxInstancesForFactory, maxInstancesForQueue, queue, constraintKeys, inputData, memoryOnly, priority); + return new Builder(id, createTime, lifespan, maxAttempts, maxInstancesForFactory, maxInstancesForQueue, queue, constraintKeys, inputData, memoryOnly, globalPriority, queuePriority); } @@ -364,14 +378,15 @@ public abstract class Job { private List constraintKeys; private byte[] inputData; private boolean memoryOnly; - private int priority; + private int globalPriority; + private int queuePriority; public Builder() { this(UUID.randomUUID().toString()); } Builder(@NonNull String id) { - this(id, System.currentTimeMillis(), IMMORTAL, 1, UNLIMITED, UNLIMITED, null, new LinkedList<>(), null, false, Parameters.PRIORITY_DEFAULT); + this(id, System.currentTimeMillis(), IMMORTAL, 1, UNLIMITED, UNLIMITED, null, new LinkedList<>(), null, false, Parameters.PRIORITY_DEFAULT, Parameters.PRIORITY_DEFAULT); } private Builder(@NonNull String id, @@ -384,7 +399,8 @@ public abstract class Job { @NonNull List constraintKeys, @Nullable byte[] inputData, boolean memoryOnly, - int priority) + int globalPriority, + int queuePriority) { this.id = id; this.createTime = createTime; @@ -396,7 +412,8 @@ public abstract class Job { this.constraintKeys = constraintKeys; this.inputData = inputData; this.memoryOnly = memoryOnly; - this.priority = priority; + this.globalPriority = globalPriority; + this.queuePriority = queuePriority; } /** Should only be invoked by {@link JobController} */ @@ -491,7 +508,7 @@ public abstract class Job { } /** - * Sets the job's priority. Higher numbers are higher priority. Use the constants {@link Parameters#PRIORITY_HIGH}, {@link Parameters#PRIORITY_LOW}, + * Sets the job's global priority. Higher numbers are higher priority. Use the constants {@link Parameters#PRIORITY_HIGH}, {@link Parameters#PRIORITY_LOW}, * and {@link Parameters#PRIORITY_DEFAULT}. Defaults to {@link Parameters#PRIORITY_DEFAULT}. * * Priority determines the order jobs are run. In general, higher priority jobs run first. When deciding which job to run within a queue, we will always @@ -505,8 +522,25 @@ public abstract class Job { * of this, as it provides the potential for lower-priority jobs to be extremely delayed if higher-priority jobs are being consistently enqueued at the * same time. */ - public @NonNull Builder setPriority(int priority) { - this.priority = priority; + public @NonNull Builder setGlobalPriority(@Priority int priority) { + this.globalPriority = priority; + return this; + } + + /** + * Sets the job's queue priority. Higher numbers are higher priority. Use the constants {@link Parameters#PRIORITY_HIGH}, {@link Parameters#PRIORITY_LOW}, + * and {@link Parameters#PRIORITY_DEFAULT}. Defaults to {@link Parameters#PRIORITY_DEFAULT}. + * + * Queue priority determines the order jobs are run within a queue. It's a secondary attribute to {@link #setGlobalPriority(int)}. When deciding which job + * to run within a queue, if two jobs have equal global priorities, we will always run the oldest job that has the highest queue priority. For example, + * if the highest queue priority in the queue is {@link Parameters#PRIORITY_DEFAULT} (and all global priorities are equal), then we'll run the oldest job + * with that queue priority, ignoring lower-priority jobs. + * + * Outside of picking the "most eligible job" within a queue, the queue priority is not used. It is ignored when choosing which job to run amongst + * multiple queues. If you'd like to influence that, see {@link #setGlobalPriority(int)}. + */ + public @NonNull Builder setQueuePriority(@Priority int priority) { + this.queuePriority = priority; return this; } @@ -520,7 +554,7 @@ public abstract class Job { } public @NonNull Parameters build() { - return new Parameters(id, createTime, lifespan, maxAttempts, maxInstancesForFactory, maxInstancesForQueue, queue, constraintKeys, inputData, memoryOnly, priority); + return new Parameters(id, createTime, lifespan, maxAttempts, maxInstancesForFactory, maxInstancesForQueue, queue, constraintKeys, inputData, memoryOnly, globalPriority, queuePriority); } } } diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobmanager/JobController.java b/app/src/main/java/org/thoughtcrime/securesms/jobmanager/JobController.java index fd87e14684..c26045450f 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobmanager/JobController.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobmanager/JobController.java @@ -448,7 +448,8 @@ class JobController { null, false, job.getParameters().isMemoryOnly(), - job.getParameters().getPriority()); + job.getParameters().getGlobalPriority(), + job.getParameters().getQueuePriority()); List constraintSpecs = Stream.of(job.getParameters().getConstraintKeys()) .map(key -> new ConstraintSpec(jobSpec.getId(), key, jobSpec.isMemoryOnly())) @@ -556,7 +557,8 @@ class JobController { inputData, jobSpec.isRunning(), jobSpec.isMemoryOnly(), - jobSpec.getPriority()); + jobSpec.getGlobalPriority(), + jobSpec.getQueuePriority()); } interface Callback { diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobmanager/JobMigrator.java b/app/src/main/java/org/thoughtcrime/securesms/jobmanager/JobMigrator.java index 60ef3da6bc..bb4706ea21 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobmanager/JobMigrator.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobmanager/JobMigrator.java @@ -72,7 +72,8 @@ public class JobMigrator { jobSpec.getSerializedInputData(), jobSpec.isRunning(), jobSpec.isMemoryOnly(), - jobSpec.getPriority()); + jobSpec.getGlobalPriority(), + jobSpec.getQueuePriority()); }); } diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobmanager/persistence/JobSpec.kt b/app/src/main/java/org/thoughtcrime/securesms/jobmanager/persistence/JobSpec.kt index 498b49d342..491991904b 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobmanager/persistence/JobSpec.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobmanager/persistence/JobSpec.kt @@ -14,7 +14,8 @@ data class JobSpec( val serializedInputData: ByteArray?, val isRunning: Boolean, val isMemoryOnly: Boolean, - val priority: Int + val globalPriority: Int, + val queuePriority: Int ) { fun withNextBackoffInterval(updated: Long): JobSpec { @@ -26,7 +27,7 @@ data class JobSpec( } override fun toString(): String { - return "id: JOB::$id | factoryKey: $factoryKey | queueKey: $queueKey | createTime: $createTime | lastRunAttemptTime: $lastRunAttemptTime | nextBackoffInterval: $nextBackoffInterval | runAttempt: $runAttempt | maxAttempts: $maxAttempts | lifespan: $lifespan | isRunning: $isRunning | memoryOnly: $isMemoryOnly" + return "id: JOB::$id | factoryKey: $factoryKey | queueKey: $queueKey | createTime: $createTime | lastRunAttemptTime: $lastRunAttemptTime | nextBackoffInterval: $nextBackoffInterval | runAttempt: $runAttempt | maxAttempts: $maxAttempts | lifespan: $lifespan | isRunning: $isRunning | memoryOnly: $isMemoryOnly | globalPriority: $globalPriority | queuePriorty: $queuePriority" } override fun equals(other: Any?): Boolean { diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/ConversationShortcutUpdateJob.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/ConversationShortcutUpdateJob.java index ea033062aa..ee473ba65a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/ConversationShortcutUpdateJob.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/ConversationShortcutUpdateJob.java @@ -38,7 +38,7 @@ public class ConversationShortcutUpdateJob extends BaseJob { .setQueue("ConversationShortcutUpdateJob") .setLifespan(TimeUnit.MINUTES.toMillis(15)) .setMaxInstancesForFactory(1) - .setPriority(Parameters.PRIORITY_LOW) + .setGlobalPriority(Parameters.PRIORITY_LOW) .build()); } diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/FastJobStorage.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/FastJobStorage.kt index 60453d6a86..2c6657273f 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/FastJobStorage.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/FastJobStorage.kt @@ -396,7 +396,7 @@ class FastJobStorage(private val jobDatabase: JobDatabase) : JobStorage { createTime = updated.createTime, lastRunAttemptTime = updated.lastRunAttemptTime, nextBackoffInterval = updated.nextBackoffInterval, - priority = updated.priority, + globalPriority = updated.globalPriority, isRunning = updated.isRunning, isMemoryOnly = updated.isMemoryOnly ) @@ -413,35 +413,36 @@ class FastJobStorage(private val jobDatabase: JobDatabase) : JobStorage { /** * Heart of a lot of the in-memory job management. Will ensure that we have an up-to-date list of eligible jobs in sorted order. */ - private fun placeJobInEligibleList(job: MinimalJobSpec) { - var jobToPlace: MinimalJobSpec? = job + private fun placeJobInEligibleList(jobCandidate: MinimalJobSpec) { + val existingJobInQueue = jobCandidate.queueKey?.let { mostEligibleJobForQueue[it] } + if (existingJobInQueue != null) { + if (jobCandidate.globalPriority < existingJobInQueue.globalPriority) { + return + } - if (job.queueKey != null) { - val existingJobInQueue = mostEligibleJobForQueue[job.queueKey] - if (existingJobInQueue != null) { - // We only want a single job from each queue. It should be the oldest job with the highest priority. - if (job.priority > existingJobInQueue.priority || (job.priority == existingJobInQueue.priority && job.createTime < existingJobInQueue.createTime)) { - mostEligibleJobForQueue[job.queueKey] = job - eligibleJobs.removeIf { it.id == existingJobInQueue.id } - } else { - // There's a more eligible job in the queue already, so no need to put it in the eligible list - jobToPlace = null + if (jobCandidate.globalPriority == existingJobInQueue.globalPriority) { + if (jobCandidate.queuePriority < existingJobInQueue.queuePriority) { + return + } + + if (jobCandidate.queuePriority == existingJobInQueue.queuePriority && jobCandidate.createTime >= existingJobInQueue.createTime) { + return } } } - if (jobToPlace == null) { - return - } + // At this point, we know that the job candidate has a higher global priority, higher queue priority, or their priorities are the same but with an older creation time. + // That means we know it's now the most eligible job in its queue. - jobToPlace.queueKey?.let { queueKey -> - mostEligibleJobForQueue[queueKey] = job + jobCandidate.queueKey?.let { queueKey -> + eligibleJobs.removeIf { it.id == existingJobInQueue?.id } + mostEligibleJobForQueue[queueKey] = jobCandidate } // At this point, anything queue-related has been handled. We just need to insert this job in the correct spot in the list. // Thankfully, we're using a TreeSet, so sorting is automatic. - eligibleJobs += jobToPlace + eligibleJobs += jobCandidate } /** @@ -522,14 +523,15 @@ class FastJobStorage(private val jobDatabase: JobDatabase) : JobStorage { private object EligibleMinJobComparator : Comparator { override fun compare(o1: MinimalJobSpec, o2: MinimalJobSpec): Int { - // We want to sort by priority descending, then createTime ascending + // We want to sort by priority descending, then createTime ascending. + // This is for determining which job to run across multiple queues, so queue priority is not considered. // CAUTION: This is used by a TreeSet, so it must be consistent with equals. // If this compare function says two objects are equal, then only one will be allowed in the set! // This is why the last step is to compare the IDs. return when { - o1.priority > o2.priority -> -1 - o1.priority < o2.priority -> 1 + o1.globalPriority > o2.globalPriority -> -1 + o1.globalPriority < o2.globalPriority -> 1 o1.createTime < o2.createTime -> -1 o1.createTime > o2.createTime -> 1 else -> o1.id.compareTo(o2.id) @@ -543,8 +545,8 @@ class FastJobStorage(private val jobDatabase: JobDatabase) : JobStorage { private object EligibleFullJobComparator : Comparator { override fun compare(o1: JobSpec, o2: JobSpec): Int { return when { - o1.priority > o2.priority -> -1 - o1.priority < o2.priority -> 1 + o1.globalPriority > o2.globalPriority -> -1 + o1.globalPriority < o2.globalPriority -> 1 o1.createTime < o2.createTime -> -1 o1.createTime > o2.createTime -> 1 else -> o1.id.compareTo(o2.id) @@ -569,7 +571,8 @@ fun JobSpec.toMinimalJobSpec(): MinimalJobSpec { createTime = this.createTime, lastRunAttemptTime = this.lastRunAttemptTime, nextBackoffInterval = this.nextBackoffInterval, - priority = this.priority, + globalPriority = this.globalPriority, + queuePriority = this.queuePriority, isRunning = this.isRunning, isMemoryOnly = this.isMemoryOnly ) diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/MinimalJobSpec.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/MinimalJobSpec.kt index f95a3bb03f..54351103c0 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/MinimalJobSpec.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/MinimalJobSpec.kt @@ -16,7 +16,8 @@ data class MinimalJobSpec( val createTime: Long, val lastRunAttemptTime: Long, val nextBackoffInterval: Long, - val priority: Int, + val globalPriority: Int, + val queuePriority: Int, val isRunning: Boolean, val isMemoryOnly: Boolean ) diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/PushGroupSilentUpdateSendJob.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/PushGroupSilentUpdateSendJob.java index 321c191da0..9c24ba0b6a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/PushGroupSilentUpdateSendJob.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/PushGroupSilentUpdateSendJob.java @@ -98,7 +98,7 @@ public final class PushGroupSilentUpdateSendJob extends BaseJob { .setQueue(queue) .setLifespan(TimeUnit.DAYS.toMillis(1)) .setMaxAttempts(Parameters.UNLIMITED) - .setPriority(Parameters.PRIORITY_LOW) + .setGlobalPriority(Parameters.PRIORITY_LOW) .build()); } diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/RestoreAttachmentThumbnailJob.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/RestoreAttachmentThumbnailJob.kt index 78592108ce..f8b60468d8 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/RestoreAttachmentThumbnailJob.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/RestoreAttachmentThumbnailJob.kt @@ -56,7 +56,7 @@ class RestoreAttachmentThumbnailJob private constructor( .addConstraint(NetworkConstraint.KEY) .setLifespan(TimeUnit.DAYS.toMillis(1)) .setMaxAttempts(Parameters.UNLIMITED) - .setPriority(if (highPriority) Parameters.PRIORITY_HIGH else Parameters.PRIORITY_DEFAULT) + .setGlobalPriority(if (highPriority) Parameters.PRIORITY_HIGH else Parameters.PRIORITY_DEFAULT) .build(), messageId, attachmentId diff --git a/app/src/test/java/org/thoughtcrime/securesms/jobmanager/JobMigratorTest.java b/app/src/test/java/org/thoughtcrime/securesms/jobmanager/JobMigratorTest.java index f8a4a61ae1..d713666152 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/jobmanager/JobMigratorTest.java +++ b/app/src/test/java/org/thoughtcrime/securesms/jobmanager/JobMigratorTest.java @@ -92,7 +92,7 @@ public class JobMigratorTest { private static JobStorage simpleJobStorage() { JobStorage jobStorage = mock(JobStorage.class); - JobSpec job = new JobSpec("1", "f1", null, 1, 1, 1, 1, 1, 1, null, null, false, false, 0); + JobSpec job = new JobSpec("1", "f1", null, 1, 1, 1, 1, 1, 1, null, null, false, false, 0, 0); when(jobStorage.debugGetJobSpecs(anyInt())).thenReturn(new ArrayList<>(Collections.singletonList(job))); doAnswer(invocation -> { diff --git a/app/src/test/java/org/thoughtcrime/securesms/jobs/FastJobStorageTest.kt b/app/src/test/java/org/thoughtcrime/securesms/jobs/FastJobStorageTest.kt index f4fdef4a3f..1d970aac34 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/jobs/FastJobStorageTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/jobs/FastJobStorageTest.kt @@ -414,10 +414,10 @@ class FastJobStorageTest { } @Test - fun `getNextEligibleJob - first item in queue with priority`() { - val fullSpec1 = FullSpec(jobSpec(id = "1", factoryKey = "f1", queueKey = "q", createTime = 1, priority = Job.Parameters.PRIORITY_LOW), emptyList(), emptyList()) - val fullSpec2 = FullSpec(jobSpec(id = "2", factoryKey = "f2", queueKey = "q", createTime = 2, priority = Job.Parameters.PRIORITY_HIGH), emptyList(), emptyList()) - val fullSpec3 = FullSpec(jobSpec(id = "3", factoryKey = "f3", queueKey = "q", createTime = 3, priority = Job.Parameters.PRIORITY_DEFAULT), emptyList(), emptyList()) + fun `getNextEligibleJob - first item in queue with global priority`() { + val fullSpec1 = FullSpec(jobSpec(id = "1", factoryKey = "f1", queueKey = "q", createTime = 1, globalPriority = Job.Parameters.PRIORITY_LOW), emptyList(), emptyList()) + val fullSpec2 = FullSpec(jobSpec(id = "2", factoryKey = "f2", queueKey = "q", createTime = 2, globalPriority = Job.Parameters.PRIORITY_HIGH), emptyList(), emptyList()) + val fullSpec3 = FullSpec(jobSpec(id = "3", factoryKey = "f3", queueKey = "q", createTime = 3, globalPriority = Job.Parameters.PRIORITY_DEFAULT), emptyList(), emptyList()) val subject = FastJobStorage(mockDatabase(listOf(fullSpec1, fullSpec2, fullSpec3))) subject.init() @@ -428,18 +428,21 @@ class FastJobStorageTest { } @Test - fun `getNextEligibleJob - complex priority`() { - val fullSpec1 = FullSpec(jobSpec(id = "1", factoryKey = "f1", queueKey = "q1", createTime = 1, priority = Job.Parameters.PRIORITY_LOW), emptyList(), emptyList()) - val fullSpec2 = FullSpec(jobSpec(id = "2", factoryKey = "f2", queueKey = "q1", createTime = 2, priority = Job.Parameters.PRIORITY_HIGH), emptyList(), emptyList()) - val fullSpec3 = FullSpec(jobSpec(id = "3", factoryKey = "f3", queueKey = "q2", createTime = 3, priority = Job.Parameters.PRIORITY_DEFAULT), emptyList(), emptyList()) - val fullSpec4 = FullSpec(jobSpec(id = "4", factoryKey = "f4", queueKey = "q2", createTime = 4, priority = Job.Parameters.PRIORITY_LOW), emptyList(), emptyList()) - val fullSpec5 = FullSpec(jobSpec(id = "5", factoryKey = "f5", queueKey = "q3", createTime = 5, priority = Job.Parameters.PRIORITY_DEFAULT), emptyList(), emptyList()) - val fullSpec6 = FullSpec(jobSpec(id = "6", factoryKey = "f6", queueKey = "q3", createTime = 6, priority = Job.Parameters.PRIORITY_HIGH), emptyList(), emptyList()) - val fullSpec7 = FullSpec(jobSpec(id = "7", factoryKey = "f7", queueKey = "q4", createTime = 7, priority = Job.Parameters.PRIORITY_LOW), emptyList(), emptyList()) - val fullSpec8 = FullSpec(jobSpec(id = "8", factoryKey = "f8", queueKey = null, createTime = 8, priority = Job.Parameters.PRIORITY_LOW), emptyList(), emptyList()) - val fullSpec9 = FullSpec(jobSpec(id = "9", factoryKey = "f9", queueKey = null, createTime = 9, priority = Job.Parameters.PRIORITY_DEFAULT), emptyList(), emptyList()) + fun `getNextEligibleJob - complex global priority`() { + val fullSpec1 = FullSpec(jobSpec(id = "1", factoryKey = "f1", queueKey = "q1", createTime = 1, globalPriority = Job.Parameters.PRIORITY_LOW, queuePriority = Job.Parameters.PRIORITY_DEFAULT), emptyList(), emptyList()) + val fullSpec2 = FullSpec(jobSpec(id = "2", factoryKey = "f2", queueKey = "q1", createTime = 2, globalPriority = Job.Parameters.PRIORITY_HIGH, queuePriority = Job.Parameters.PRIORITY_DEFAULT), emptyList(), emptyList()) + val fullSpec3 = FullSpec(jobSpec(id = "3", factoryKey = "f3", queueKey = "q2", createTime = 3, globalPriority = Job.Parameters.PRIORITY_DEFAULT, queuePriority = Job.Parameters.PRIORITY_DEFAULT), emptyList(), emptyList()) + val fullSpec4 = FullSpec(jobSpec(id = "4", factoryKey = "f4", queueKey = "q2", createTime = 4, globalPriority = Job.Parameters.PRIORITY_LOW, queuePriority = Job.Parameters.PRIORITY_DEFAULT), emptyList(), emptyList()) + val fullSpec5 = FullSpec(jobSpec(id = "5", factoryKey = "f5", queueKey = "q3", createTime = 5, globalPriority = Job.Parameters.PRIORITY_DEFAULT, queuePriority = Job.Parameters.PRIORITY_DEFAULT), emptyList(), emptyList()) + val fullSpec6 = FullSpec(jobSpec(id = "6", factoryKey = "f6", queueKey = "q3", createTime = 6, globalPriority = Job.Parameters.PRIORITY_HIGH, queuePriority = Job.Parameters.PRIORITY_DEFAULT), emptyList(), emptyList()) + val fullSpec7 = FullSpec(jobSpec(id = "7", factoryKey = "f7", queueKey = "q4", createTime = 7, globalPriority = Job.Parameters.PRIORITY_LOW, queuePriority = Job.Parameters.PRIORITY_DEFAULT), emptyList(), emptyList()) + val fullSpec8 = FullSpec(jobSpec(id = "8", factoryKey = "f8", queueKey = null, createTime = 8, globalPriority = Job.Parameters.PRIORITY_LOW, queuePriority = Job.Parameters.PRIORITY_DEFAULT), emptyList(), emptyList()) + val fullSpec9 = FullSpec(jobSpec(id = "9", factoryKey = "f9", queueKey = null, createTime = 9, globalPriority = Job.Parameters.PRIORITY_DEFAULT, queuePriority = Job.Parameters.PRIORITY_DEFAULT), emptyList(), emptyList()) + val fullSpec10 = FullSpec(jobSpec(id = "10", factoryKey = "f10", queueKey = "q5", createTime = 10, globalPriority = Job.Parameters.PRIORITY_HIGH, queuePriority = Job.Parameters.PRIORITY_DEFAULT), emptyList(), emptyList()) + val fullSpec11 = FullSpec(jobSpec(id = "11", factoryKey = "f11", queueKey = "q5", createTime = 11, globalPriority = Job.Parameters.PRIORITY_HIGH, queuePriority = Job.Parameters.PRIORITY_HIGH), emptyList(), emptyList()) + val fullSpec12 = FullSpec(jobSpec(id = "12", factoryKey = "f12", queueKey = "q5", createTime = 12, globalPriority = Job.Parameters.PRIORITY_HIGH, queuePriority = Job.Parameters.PRIORITY_LOW), emptyList(), emptyList()) - val subject = FastJobStorage(mockDatabase(listOf(fullSpec1, fullSpec2, fullSpec3, fullSpec4, fullSpec5, fullSpec6, fullSpec7, fullSpec8, fullSpec9))) + val subject = FastJobStorage(mockDatabase(listOf(fullSpec1, fullSpec2, fullSpec3, fullSpec4, fullSpec5, fullSpec6, fullSpec7, fullSpec8, fullSpec9, fullSpec10, fullSpec11, fullSpec12))) subject.init() subject.getNextEligibleJob(10, NO_PREDICATE) assertIs fullSpec2.jobSpec @@ -448,6 +451,15 @@ class FastJobStorageTest { subject.getNextEligibleJob(10, NO_PREDICATE) assertIs fullSpec6.jobSpec subject.deleteJob(fullSpec6.jobSpec.id) + subject.getNextEligibleJob(10, NO_PREDICATE) assertIs fullSpec11.jobSpec + subject.deleteJob(fullSpec11.jobSpec.id) + + subject.getNextEligibleJob(10, NO_PREDICATE) assertIs fullSpec10.jobSpec + subject.deleteJob(fullSpec10.jobSpec.id) + + subject.getNextEligibleJob(10, NO_PREDICATE) assertIs fullSpec12.jobSpec + subject.deleteJob(fullSpec12.jobSpec.id) + subject.getNextEligibleJob(10, NO_PREDICATE) assertIs fullSpec3.jobSpec subject.deleteJob(fullSpec3.jobSpec.id) @@ -469,6 +481,33 @@ class FastJobStorageTest { subject.getNextEligibleJob(10, NO_PREDICATE) assertIs fullSpec8.jobSpec } + @Test + fun `getNextEligibleJob - first item in queue with queue priority`() { + val fullSpec1 = FullSpec(jobSpec(id = "1", factoryKey = "f1", queueKey = "q", createTime = 1, queuePriority = Job.Parameters.PRIORITY_LOW), emptyList(), emptyList()) + val fullSpec2 = FullSpec(jobSpec(id = "2", factoryKey = "f2", queueKey = "q", createTime = 2, queuePriority = Job.Parameters.PRIORITY_HIGH), emptyList(), emptyList()) + val fullSpec3 = FullSpec(jobSpec(id = "3", factoryKey = "f3", queueKey = "q", createTime = 3, queuePriority = Job.Parameters.PRIORITY_DEFAULT), emptyList(), emptyList()) + + val subject = FastJobStorage(mockDatabase(listOf(fullSpec1, fullSpec2, fullSpec3))) + subject.init() + + val job = subject.getNextEligibleJob(10, NO_PREDICATE) + job.assertIsNotNull() + job.id assertIs fullSpec2.jobSpec.id + } + + @Test + fun `getNextEligibleJob - global priority beats queue priority`() { + val fullSpec1 = FullSpec(jobSpec(id = "2", factoryKey = "f2", queueKey = "q", createTime = 2, globalPriority = Job.Parameters.PRIORITY_DEFAULT, queuePriority = Job.Parameters.PRIORITY_HIGH), emptyList(), emptyList()) + val fullSpec2 = FullSpec(jobSpec(id = "1", factoryKey = "f1", queueKey = "q", createTime = 1, globalPriority = Job.Parameters.PRIORITY_HIGH, queuePriority = Job.Parameters.PRIORITY_LOW), emptyList(), emptyList()) + + val subject = FastJobStorage(mockDatabase(listOf(fullSpec1, fullSpec2))) + subject.init() + + val job = subject.getNextEligibleJob(10, NO_PREDICATE) + job.assertIsNotNull() + job.id assertIs fullSpec2.jobSpec.id + } + @Test fun `getNextEligibleJob - lastRunAttemptTime in the future runs right away`() { val currentTime = 10L @@ -576,6 +615,60 @@ class FastJobStorageTest { subject.getNextEligibleJob(100, NO_PREDICATE) assertIs secondJob } + @Test + fun `getNextEligibleJob - after deleted, next item in queue is eligible, with global priorities`() { + // Two jobs in the same queue but with different create times + val firstJob = DataSet1.JOB_1 + val secondJob = DataSet1.JOB_1.copy(id = "id2", createTime = 2, globalPriority = Job.Parameters.PRIORITY_HIGH) + val thirdJob = DataSet1.JOB_1.copy(id = "id3", createTime = 3, globalPriority = Job.Parameters.PRIORITY_HIGH) + val subject = FastJobStorage( + mockDatabase( + fullSpecs = listOf( + FullSpec(jobSpec = firstJob, constraintSpecs = emptyList(), dependencySpecs = emptyList()), + FullSpec(jobSpec = secondJob, constraintSpecs = emptyList(), dependencySpecs = emptyList()), + FullSpec(jobSpec = thirdJob, constraintSpecs = emptyList(), dependencySpecs = emptyList()) + ) + ) + ) + subject.init() + + subject.getNextEligibleJob(100, NO_PREDICATE) assertIs secondJob + subject.deleteJob(secondJob.id) + + subject.getNextEligibleJob(100, NO_PREDICATE) assertIs thirdJob + subject.deleteJob(thirdJob.id) + + subject.getNextEligibleJob(100, NO_PREDICATE) assertIs firstJob + subject.deleteJob(firstJob.id) + } + + @Test + fun `getNextEligibleJob - after deleted, next item in queue is eligible, with queue priorities`() { + // Two jobs in the same queue but with different create times + val firstJob = DataSet1.JOB_1 + val secondJob = DataSet1.JOB_1.copy(id = "id2", createTime = 2, queuePriority = Job.Parameters.PRIORITY_HIGH) + val thirdJob = DataSet1.JOB_1.copy(id = "id3", createTime = 3, queuePriority = Job.Parameters.PRIORITY_HIGH) + val subject = FastJobStorage( + mockDatabase( + fullSpecs = listOf( + FullSpec(jobSpec = firstJob, constraintSpecs = emptyList(), dependencySpecs = emptyList()), + FullSpec(jobSpec = secondJob, constraintSpecs = emptyList(), dependencySpecs = emptyList()), + FullSpec(jobSpec = thirdJob, constraintSpecs = emptyList(), dependencySpecs = emptyList()) + ) + ) + ) + subject.init() + + subject.getNextEligibleJob(100, NO_PREDICATE) assertIs secondJob + subject.deleteJob(secondJob.id) + + subject.getNextEligibleJob(100, NO_PREDICATE) assertIs thirdJob + subject.deleteJob(thirdJob.id) + + subject.getNextEligibleJob(100, NO_PREDICATE) assertIs firstJob + subject.deleteJob(firstJob.id) + } + @Test fun `getNextEligibleJob - after marked running, no longer is in eligible list`() { val subject = FastJobStorage(mockDatabase(DataSet1.FULL_SPECS)) @@ -623,29 +716,58 @@ class FastJobStorageTest { } @Test - fun `getNextEligibleJob - newly-inserted higher-priority job in queue replaces old`() { + fun `getNextEligibleJob - newly-inserted higher-global-priority job in queue replaces old`() { val subject = FastJobStorage(mockDatabase(DataSet1.FULL_SPECS)) subject.init() subject.getNextEligibleJob(100, NO_PREDICATE) assertIs DataSet1.JOB_1 - val higherPriorityJob = DataSet1.JOB_1.copy(id = "id-bigboi", priority = Job.Parameters.PRIORITY_HIGH) + val higherPriorityJob = DataSet1.JOB_1.copy(id = "id-bigboi", globalPriority = Job.Parameters.PRIORITY_HIGH) subject.insertJobs(listOf(FullSpec(jobSpec = higherPriorityJob, constraintSpecs = emptyList(), dependencySpecs = emptyList()))) subject.getNextEligibleJob(100, NO_PREDICATE) assertIs higherPriorityJob } @Test - fun `getNextEligibleJob - updating job to have a higher priority replaces lower priority in queue`() { + fun `getNextEligibleJob - newly-inserted higher-queue-priority job in queue replaces old`() { val subject = FastJobStorage(mockDatabase(DataSet1.FULL_SPECS)) subject.init() - val lowerPriorityJob = DataSet1.JOB_1.copy(id = "id-bigboi", priority = Job.Parameters.PRIORITY_LOW) + subject.getNextEligibleJob(100, NO_PREDICATE) assertIs DataSet1.JOB_1 + + val higherPriorityJob = DataSet1.JOB_1.copy(id = "id-bigboi", queuePriority = Job.Parameters.PRIORITY_HIGH) + subject.insertJobs(listOf(FullSpec(jobSpec = higherPriorityJob, constraintSpecs = emptyList(), dependencySpecs = emptyList()))) + + subject.getNextEligibleJob(100, NO_PREDICATE) assertIs higherPriorityJob + } + + @Test + fun `getNextEligibleJob - updating job to have a higher global priority replaces lower priority in queue`() { + val subject = FastJobStorage(mockDatabase(DataSet1.FULL_SPECS)) + subject.init() + + val lowerPriorityJob = DataSet1.JOB_1.copy(id = "id-bigboi", globalPriority = Job.Parameters.PRIORITY_LOW) subject.insertJobs(listOf(FullSpec(jobSpec = lowerPriorityJob, constraintSpecs = emptyList(), dependencySpecs = emptyList()))) subject.getNextEligibleJob(100, NO_PREDICATE) assertIs DataSet1.JOB_1 - val higherPriorityJob = lowerPriorityJob.copy(priority = Job.Parameters.PRIORITY_HIGH) + val higherPriorityJob = lowerPriorityJob.copy(globalPriority = Job.Parameters.PRIORITY_HIGH) + subject.updateJobs(listOf(higherPriorityJob)) + + subject.getNextEligibleJob(100, NO_PREDICATE) assertIs higherPriorityJob + } + + @Test + fun `getNextEligibleJob - updating job to have a queue priority replaces lower priority in queue`() { + val subject = FastJobStorage(mockDatabase(DataSet1.FULL_SPECS)) + subject.init() + + val lowerPriorityJob = DataSet1.JOB_1.copy(id = "id-bigboi", queuePriority = Job.Parameters.PRIORITY_LOW) + subject.insertJobs(listOf(FullSpec(jobSpec = lowerPriorityJob, constraintSpecs = emptyList(), dependencySpecs = emptyList()))) + + subject.getNextEligibleJob(100, NO_PREDICATE) assertIs DataSet1.JOB_1 + + val higherPriorityJob = lowerPriorityJob.copy(queuePriority = Job.Parameters.PRIORITY_HIGH) subject.updateJobs(listOf(higherPriorityJob)) subject.getNextEligibleJob(100, NO_PREDICATE) assertIs higherPriorityJob @@ -887,8 +1009,10 @@ class FastJobStorageTest { every { mock.getMostEligibleJobInQueue(any()) } answers { jobs .filter { it.queueKey == firstArg() } - .sortedByDescending { it.priority } - .minByOrNull { it.createTime } + .sortedBy { it.createTime } + .sortedByDescending { it.queuePriority } + .sortedByDescending { it.globalPriority } + .firstOrNull() } return mock @@ -908,7 +1032,8 @@ class FastJobStorageTest { serializedInputData: ByteArray? = null, isRunning: Boolean = false, isMemoryOnly: Boolean = false, - priority: Int = 0 + globalPriority: Int = 0, + queuePriority: Int = 0 ): JobSpec { return JobSpec( id = id, @@ -924,7 +1049,8 @@ class FastJobStorageTest { serializedInputData = serializedInputData, isRunning = isRunning, isMemoryOnly = isMemoryOnly, - priority = priority + globalPriority = globalPriority, + queuePriority = queuePriority ) } @@ -943,7 +1069,8 @@ class FastJobStorageTest { serializedInputData = null, isRunning = false, isMemoryOnly = false, - priority = 0 + globalPriority = 0, + queuePriority = 0 ) val JOB_2 = JobSpec( id = "id2", @@ -959,7 +1086,8 @@ class FastJobStorageTest { serializedInputData = null, isRunning = false, isMemoryOnly = false, - priority = 0 + globalPriority = 0, + queuePriority = 0 ) val JOB_3 = JobSpec( id = "id3", @@ -975,7 +1103,8 @@ class FastJobStorageTest { serializedInputData = null, isRunning = false, isMemoryOnly = false, - priority = 0 + globalPriority = 0, + queuePriority = 0 ) val CONSTRAINT_1 = ConstraintSpec(jobSpecId = "id1", factoryKey = "f1", isMemoryOnly = false) @@ -1023,7 +1152,8 @@ class FastJobStorageTest { serializedInputData = null, isRunning = false, isMemoryOnly = true, - priority = 0 + globalPriority = 0, + queuePriority = 0 ) val CONSTRAINT_1 = ConstraintSpec(jobSpecId = "id1", factoryKey = "f1", isMemoryOnly = true) val FULL_SPEC_1 = FullSpec(JOB_1, listOf(CONSTRAINT_1), emptyList()) @@ -1045,7 +1175,8 @@ class FastJobStorageTest { serializedInputData = null, isRunning = false, isMemoryOnly = false, - priority = 0 + globalPriority = 0, + queuePriority = 0 ) val JOB_2 = JobSpec( id = "id2", @@ -1061,7 +1192,8 @@ class FastJobStorageTest { serializedInputData = null, isRunning = false, isMemoryOnly = false, - priority = 0 + globalPriority = 0, + queuePriority = 0 ) val JOB_3 = JobSpec( id = "id3", @@ -1077,7 +1209,8 @@ class FastJobStorageTest { serializedInputData = null, isRunning = false, isMemoryOnly = false, - priority = 0 + globalPriority = 0, + queuePriority = 0 ) val DEPENDENCY_1 = DependencySpec(jobId = "id1", dependsOnJobId = "id2", isMemoryOnly = false)