-
Notifications
You must be signed in to change notification settings - Fork 71
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
Remove MongoJack and consolidate MongoDB utils #837
base: dev
Are you sure you want to change the base?
Changes from all commits
aa1b9cd
bd77ee5
a929020
8f4a40c
efb4ec5
f5c632f
de55f9f
6b3fd5e
44e2d47
325eea9
06e7cb4
ea12ed3
341f317
b29a750
8d215b7
7546f20
71eedc0
c4b9ca9
486abb7
d5c1620
8e1a0d1
7d9a45e
d885ba6
3f4bb12
5a3d49d
9ac1659
1dfc172
432d961
b66a4be
5187485
c2ae60b
423f05e
29ed72b
de9ed5f
dd3e49a
1c4e3a6
7a0b1bf
7865c74
99e848d
552b3d7
b7eedd9
12d76ef
378e8a1
02f9319
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,17 +6,11 @@ | |
import com.conveyal.analysis.components.eventbus.EventBus; | ||
import com.conveyal.analysis.components.eventbus.RegionalAnalysisEvent; | ||
import com.conveyal.analysis.components.eventbus.WorkerEvent; | ||
import com.conveyal.analysis.models.RegionalAnalysis; | ||
import com.conveyal.analysis.results.MultiOriginAssembler; | ||
import com.conveyal.analysis.util.JsonUtil; | ||
import com.conveyal.file.FileStorage; | ||
import com.conveyal.file.FileStorageKey; | ||
import com.conveyal.file.FileUtils; | ||
import com.conveyal.r5.analyst.WorkerCategory; | ||
import com.conveyal.r5.analyst.cluster.RegionalTask; | ||
import com.conveyal.r5.analyst.cluster.RegionalWorkResult; | ||
import com.conveyal.r5.analyst.cluster.WorkerStatus; | ||
import com.conveyal.r5.analyst.scenario.Scenario; | ||
import com.conveyal.r5.util.ExceptionUtils; | ||
import com.google.common.collect.ListMultimap; | ||
import com.google.common.collect.MultimapBuilder; | ||
|
@@ -27,8 +21,6 @@ | |
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
import java.io.File; | ||
import java.io.IOException; | ||
import java.util.ArrayList; | ||
import java.util.Collection; | ||
import java.util.Collections; | ||
|
@@ -42,7 +34,6 @@ | |
import static com.conveyal.analysis.components.eventbus.WorkerEvent.Action.REQUESTED; | ||
import static com.conveyal.analysis.components.eventbus.WorkerEvent.Role.REGIONAL; | ||
import static com.conveyal.analysis.components.eventbus.WorkerEvent.Role.SINGLE_POINT; | ||
import static com.conveyal.file.FileCategory.BUNDLES; | ||
import static com.google.common.base.Preconditions.checkNotNull; | ||
|
||
/** | ||
|
@@ -93,7 +84,6 @@ public interface Config { | |
private Config config; | ||
|
||
// Component Dependencies | ||
private final FileStorage fileStorage; | ||
private final EventBus eventBus; | ||
private final WorkerLauncher workerLauncher; | ||
|
||
|
@@ -143,9 +133,8 @@ public interface Config { | |
public TObjectLongMap<WorkerCategory> recentlyRequestedWorkers = | ||
TCollections.synchronizedMap(new TObjectLongHashMap<>()); | ||
|
||
public Broker (Config config, FileStorage fileStorage, EventBus eventBus, WorkerLauncher workerLauncher) { | ||
public Broker(Config config, EventBus eventBus, WorkerLauncher workerLauncher) { | ||
this.config = config; | ||
this.fileStorage = fileStorage; | ||
this.eventBus = eventBus; | ||
this.workerLauncher = workerLauncher; | ||
} | ||
|
@@ -154,89 +143,35 @@ public Broker (Config config, FileStorage fileStorage, EventBus eventBus, Worker | |
* Enqueue a set of tasks for a regional analysis. | ||
* Only a single task is passed in, which the broker will expand into all the individual tasks for a regional job. | ||
*/ | ||
public synchronized void enqueueTasksForRegionalJob (RegionalAnalysis regionalAnalysis) { | ||
|
||
// Make a copy of the regional task inside the RegionalAnalysis, replacing the scenario with a scenario ID. | ||
RegionalTask templateTask = templateTaskFromRegionalAnalysis(regionalAnalysis); | ||
|
||
LOG.info("Enqueuing tasks for job {} using template task.", templateTask.jobId); | ||
if (findJob(templateTask.jobId) != null) { | ||
LOG.error("Someone tried to enqueue job {} but it already exists.", templateTask.jobId); | ||
throw new RuntimeException("Enqueued duplicate job " + templateTask.jobId); | ||
public synchronized void enqueueTasksForRegionalJob(Job job, MultiOriginAssembler assembler) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any synchronization concerns we should keep in mind? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not that I can think of here. This method was already |
||
// Once the assembler has been created, enqueue the job. | ||
LOG.info("Enqueuing tasks for job {} using template task.", job.jobId); | ||
if (findJob(job.jobId) != null) { | ||
LOG.error("Someone tried to enqueue job {} but it already exists.", job.jobId); | ||
throw new RuntimeException("Enqueued duplicate job " + job.jobId); | ||
} | ||
WorkerTags workerTags = WorkerTags.fromRegionalAnalysis(regionalAnalysis); | ||
Job job = new Job(templateTask, workerTags); | ||
jobs.put(job.workerCategory, job); | ||
|
||
// Register the regional job so results received from multiple workers can be assembled into one file. | ||
// TODO encapsulate MultiOriginAssemblers in a new Component | ||
// Note: if this fails with an exception we'll have a job enqueued, possibly being processed, with no assembler. | ||
// That is not catastrophic, but the user may need to recognize and delete the stalled regional job. | ||
MultiOriginAssembler assembler = new MultiOriginAssembler(regionalAnalysis, job, fileStorage); | ||
resultAssemblers.put(templateTask.jobId, assembler); | ||
resultAssemblers.put(job.jobId, assembler); | ||
|
||
if (config.testTaskRedelivery()) { | ||
// This is a fake job for testing, don't confuse the worker startup code below with null graph ID. | ||
return; | ||
} | ||
|
||
if (workerCatalog.noWorkersAvailable(job.workerCategory, config.offline())) { | ||
createOnDemandWorkerInCategory(job.workerCategory, workerTags); | ||
createOnDemandWorkerInCategory(job.workerCategory, job.workerTags); | ||
} else { | ||
// Workers exist in this category, clear out any record that we're waiting for one to start up. | ||
recentlyRequestedWorkers.remove(job.workerCategory); | ||
} | ||
eventBus.send(new RegionalAnalysisEvent(templateTask.jobId, STARTED).forUser(workerTags.user, workerTags.group)); | ||
} | ||
|
||
/** | ||
* The single RegionalTask object represents a lot of individual accessibility tasks at many different origin | ||
* points, typically on a grid. Before passing that RegionalTask on to the Broker (which distributes tasks to | ||
* workers and tracks progress), we remove the details of the scenario, substituting the scenario's unique ID | ||
* to save time and bandwidth. This avoids repeatedly sending the scenario details to the worker in every task, | ||
* as they are often quite voluminous. The workers will fetch the scenario once from S3 and cache it based on | ||
* its ID only. We protectively clone this task because we're going to null out its scenario field, and don't | ||
* want to affect the original object which contains all the scenario details. | ||
* TODO Why is all this detail added after the Persistence call? | ||
* We don't want to store all the details added below in Mongo? | ||
*/ | ||
private RegionalTask templateTaskFromRegionalAnalysis (RegionalAnalysis regionalAnalysis) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replaced by Note that scenario.json is now created and saved to S3 at |
||
RegionalTask templateTask = regionalAnalysis.request.clone(); | ||
// First replace the inline scenario with a scenario ID, storing the scenario for retrieval by workers. | ||
Scenario scenario = templateTask.scenario; | ||
templateTask.scenarioId = scenario.id; | ||
// Null out the scenario in the template task, avoiding repeated serialization to the workers as massive JSON. | ||
templateTask.scenario = null; | ||
String fileName = String.format("%s_%s.json", regionalAnalysis.bundleId, scenario.id); | ||
FileStorageKey fileStorageKey = new FileStorageKey(BUNDLES, fileName); | ||
try { | ||
File localScenario = FileUtils.createScratchFile("json"); | ||
JsonUtil.objectMapper.writeValue(localScenario, scenario); | ||
// FIXME this is using a network service in a method called from a synchronized broker method. | ||
// Move file into storage before entering the synchronized block. | ||
fileStorage.moveIntoStorage(fileStorageKey, localScenario); | ||
} catch (IOException e) { | ||
LOG.error("Error storing scenario for retrieval by workers.", e); | ||
} | ||
// Fill in all the fields in the template task that will remain the same across all tasks in a job. | ||
// I am not sure why we are re-setting all these fields, it seems like they are already set when the task is | ||
// initialized by AnalysisRequest.populateTask. But we'd want to thoroughly check that assumption before | ||
// eliminating or moving these lines. | ||
templateTask.jobId = regionalAnalysis._id; | ||
templateTask.graphId = regionalAnalysis.bundleId; | ||
templateTask.workerVersion = regionalAnalysis.workerVersion; | ||
templateTask.height = regionalAnalysis.height; | ||
templateTask.width = regionalAnalysis.width; | ||
templateTask.north = regionalAnalysis.north; | ||
templateTask.west = regionalAnalysis.west; | ||
templateTask.zoom = regionalAnalysis.zoom; | ||
return templateTask; | ||
eventBus.send(new RegionalAnalysisEvent(job.jobId, STARTED).forUser(job.workerTags.user, job.workerTags.group)); | ||
} | ||
|
||
/** | ||
* Create on-demand worker for a given job. | ||
*/ | ||
public void createOnDemandWorkerInCategory(WorkerCategory category, WorkerTags workerTags){ | ||
public void createOnDemandWorkerInCategory(WorkerCategory category, WorkerTags workerTags) { | ||
createWorkersInCategory(category, workerTags, 1, 0); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,16 @@ | ||
package com.conveyal.analysis.components.broker; | ||
|
||
import com.conveyal.analysis.UserPermissions; | ||
import com.conveyal.analysis.components.BackendComponents; | ||
import com.conveyal.analysis.components.LocalBackendComponents; | ||
import com.conveyal.analysis.models.RegionalAnalysis; | ||
import com.conveyal.analysis.results.MultiOriginAssembler; | ||
import com.conveyal.r5.analyst.cluster.RegionalTask; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
import java.nio.ByteBuffer; | ||
import java.util.ArrayList; | ||
import java.util.UUID; | ||
|
||
/** | ||
|
@@ -64,9 +67,11 @@ private static void sendFakeJob(Broker broker) { | |
templateTask.height = 1; | ||
templateTask.width = N_TASKS_PER_JOB; | ||
templateTask.scenarioId = "FAKE"; | ||
RegionalAnalysis regionalAnalysis = new RegionalAnalysis(); | ||
RegionalAnalysis regionalAnalysis = new RegionalAnalysis(new UserPermissions("[email protected]", false, "testing"), "test"); | ||
regionalAnalysis.request = templateTask; | ||
broker.enqueueTasksForRegionalJob(regionalAnalysis); | ||
var job = new Job(templateTask, WorkerTags.fromRegionalAnalysis(regionalAnalysis)); | ||
var assembler = new MultiOriginAssembler(job, new ArrayList<>()); | ||
Comment on lines
+72
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we update our style guide re: use of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Although I haven't looked at our style guide in ages. My short opinion / addition to the style guide is: prefer using In the examples above, writing the types instead of A case where we might want to be more explicit, is when a method returns a value and we want to distinguish that type in relation to neighboring types. |
||
broker.enqueueTasksForRegionalJob(job, assembler); | ||
} | ||
|
||
public static String compactUUID() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋