From 7060665fc46ce6004668727345e3847348f11a7d Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Wed, 20 Dec 2023 14:03:38 +0800 Subject: [PATCH 1/3] fix final and static field modifiers in broker Many constants (as indicated by capitalization) were not static. One field always set in the constructor was not final. The list of resultAssemblers WAS static. This may be a remnant of pre- component design. This was not problematic because we never construct more than one Broker, but it should be possible to do (no global state). --- .../analysis/components/broker/Broker.java | 23 ++++++++----------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/main/java/com/conveyal/analysis/components/broker/Broker.java b/src/main/java/com/conveyal/analysis/components/broker/Broker.java index 2aea67dc2..d9f78a6e0 100644 --- a/src/main/java/com/conveyal/analysis/components/broker/Broker.java +++ b/src/main/java/com/conveyal/analysis/components/broker/Broker.java @@ -95,15 +95,14 @@ public interface Config { boolean testTaskRedelivery (); } - private Config config; + private final Config config; // Component Dependencies private final FileStorage fileStorage; private final EventBus eventBus; private final WorkerLauncher workerLauncher; - private final ListMultimap jobs = - MultimapBuilder.hashKeys().arrayListValues().build(); + private final ListMultimap jobs = MultimapBuilder.hashKeys().arrayListValues().build(); /** * The most tasks to deliver to a worker at a time. Workers may request less tasks than this, and the broker should @@ -114,24 +113,24 @@ public interface Config { * The value should eventually be tuned. The current value of 16 is just the value used by the previous sporadic * polling system (WorkerStatus.LEGACY_WORKER_MAX_TASKS) which may not be ideal but is known to work. */ - public final int MAX_TASKS_PER_WORKER = 16; + public static final int MAX_TASKS_PER_WORKER = 16; /** * Used when auto-starting spot instances. Set to a smaller value to increase the number of * workers requested automatically */ - public final int TARGET_TASKS_PER_WORKER_TRANSIT = 800; - public final int TARGET_TASKS_PER_WORKER_NONTRANSIT = 4_000; + public static final int TARGET_TASKS_PER_WORKER_TRANSIT = 800; + public static final int TARGET_TASKS_PER_WORKER_NONTRANSIT = 4_000; /** * We want to request spot instances to "boost" regional analyses after a few regional task * results are received for a given workerCategory. Do so after receiving results for an * arbitrary task toward the beginning of the job */ - public final int AUTO_START_SPOT_INSTANCES_AT_TASK = 42; + public static final int AUTO_START_SPOT_INSTANCES_AT_TASK = 42; /** The maximum number of spot instances allowable in an automatic request */ - public final int MAX_WORKERS_PER_CATEGORY = 250; + public static final int MAX_WORKERS_PER_CATEGORY = 250; /** * How long to give workers to start up (in ms) before assuming that they have started (and @@ -139,15 +138,11 @@ public interface Config { */ public static final long WORKER_STARTUP_TIME = 60 * 60 * 1000; - /** Keeps track of all the workers that have contacted this broker recently asking for work. */ private WorkerCatalog workerCatalog = new WorkerCatalog(); - /** - * These objects piece together results received from workers into one regional analysis result - * file per job. - */ - private static Map resultAssemblers = new HashMap<>(); + /** These objects piece together results received from workers into one regional analysis result file per job. */ + private Map resultAssemblers = new HashMap<>(); /** * keep track of which graphs we have launched workers on and how long ago we launched them, so From 32eb1dc49cb27868445004df68557e914f457700 Mon Sep 17 00:00:00 2001 From: Trevor Gerhardt Date: Fri, 26 Jan 2024 11:51:44 +0100 Subject: [PATCH 2/3] Exclude modifications for regional analysis lookup (#926) This change adds a new helper method to pass a `fields` object to `MongoMap#findByIdIfPermitted` and utilizes that to exclude modifications while looking up a regional analysis to generate a `scenarioJsonUrl`. This will prevent errors from occurring if custom modifications are used in a worker version that do not exist in the server. --- .../RegionalAnalysisController.java | 7 +++++-- .../analysis/persistence/MongoMap.java | 19 +++++++++++++------ 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/conveyal/analysis/controllers/RegionalAnalysisController.java b/src/main/java/com/conveyal/analysis/controllers/RegionalAnalysisController.java index 32336fd7c..b08cb8a9b 100644 --- a/src/main/java/com/conveyal/analysis/controllers/RegionalAnalysisController.java +++ b/src/main/java/com/conveyal/analysis/controllers/RegionalAnalysisController.java @@ -527,8 +527,11 @@ private RegionalAnalysis updateRegionalAnalysis (Request request, Response respo * the given regional analysis. */ private JsonNode getScenarioJsonUrl (Request request, Response response) { - RegionalAnalysis regionalAnalysis = Persistence.regionalAnalyses - .findByIdIfPermitted(request.params("_id"), UserPermissions.from(request)); + RegionalAnalysis regionalAnalysis = Persistence.regionalAnalyses.findByIdIfPermitted( + request.params("_id"), + DBProjection.exclude("request.scenario.modifications"), + UserPermissions.from(request) + ); // In the persisted objects, regionalAnalysis.scenarioId seems to be null. Get it from the embedded request. final String networkId = regionalAnalysis.bundleId; final String scenarioId = regionalAnalysis.request.scenarioId; diff --git a/src/main/java/com/conveyal/analysis/persistence/MongoMap.java b/src/main/java/com/conveyal/analysis/persistence/MongoMap.java index 6001a82d2..4f34353d1 100644 --- a/src/main/java/com/conveyal/analysis/persistence/MongoMap.java +++ b/src/main/java/com/conveyal/analysis/persistence/MongoMap.java @@ -43,12 +43,11 @@ public int size() { return (int) wrappedCollection.getCount(); } - public V findByIdFromRequestIfPermitted(Request request) { - return findByIdIfPermitted(request.params("_id"), UserPermissions.from(request)); - } - - public V findByIdIfPermitted(String id, UserPermissions userPermissions) { - V result = wrappedCollection.findOneById(id); + /** + * `fields` is nullable. + */ + public V findByIdIfPermitted(String id, DBObject fields, UserPermissions userPermissions) { + V result = wrappedCollection.findOneById(id, fields); if (result == null) { throw AnalysisServerException.notFound(String.format( @@ -61,6 +60,14 @@ public V findByIdIfPermitted(String id, UserPermissions userPermissions) { } } + public V findByIdIfPermitted(String id, UserPermissions userPermissions) { + return findByIdIfPermitted(id, null, userPermissions); + } + + public V findByIdFromRequestIfPermitted(Request request) { + return findByIdIfPermitted(request.params("_id"), UserPermissions.from(request)); + } + public V get(String key) { return wrappedCollection.findOneById(key); } From 2376c4edd56504f9d7eec4ceb1c211b96322a2f8 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Wed, 14 Feb 2024 00:06:29 +0800 Subject: [PATCH 3/3] keepRoutingOnFoot only in transit access searches --- .../conveyal/r5/analyst/TravelTimeComputer.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/conveyal/r5/analyst/TravelTimeComputer.java b/src/main/java/com/conveyal/r5/analyst/TravelTimeComputer.java index 6af5dbcf7..0237a636e 100644 --- a/src/main/java/com/conveyal/r5/analyst/TravelTimeComputer.java +++ b/src/main/java/com/conveyal/r5/analyst/TravelTimeComputer.java @@ -167,14 +167,16 @@ public OneOriginResult computeTravelTimes() { // The generalized cost calculations currently increment time and weight by the same amount. sr.quantityToMinimize = StreetRouter.State.RoutingVariable.DURATION_SECONDS; sr.route(); - // Change to walking in order to reach transit stops in pedestrian-only areas like train stations. - // This implies you are dropped off or have a very easy parking spot for your vehicle. - // This kind of multi-stage search should also be used when building egress distance cost tables. - if (accessMode != StreetMode.WALK) { - sr.keepRoutingOnFoot(); - } if (request.hasTransit()) { + // Change to walking in order to reach transit stops in pedestrian-only areas like train stations. + // This implies you are dropped off or have a very easy parking spot for your vehicle. + // This kind of multi-stage search should also be used when building egress distance cost tables. + // Note that this can take up to twice as long as the initial car/bike search. Do it only when the + // walking is necessary, and when the radius of the car/bike search is limited, as for transit access. + if (accessMode != StreetMode.WALK) { + sr.keepRoutingOnFoot(); + } // Find access times to transit stops, keeping the minimum across all access street modes. // Note that getReachedStops() returns the routing variable units, not necessarily seconds. // TODO add logic here if linkedStops are specified in pickupDelay?