Skip to content

Commit

Permalink
fix(async-loading): remove flawed synchronous methods
Browse files Browse the repository at this point in the history
Instead of having both single point and regional tasks go through the pre-loader, the regional tasks just bypass it entirely now and hit the network cache directly.
The problem with the synchronous preloading code was that it triggered the preload process on every request, setting partial load progress even when loading had already been previously accomplished.
It then never hit the point where the loading state was PRESENT, which premanently prevented any subsequent async requests from succeeding.
The ideal was to have both kinds of tasks go through the preloader so we could eventually get rid of the old nested loadingCaches, but that's going to take more refactoring.
  • Loading branch information
abyrd committed Nov 21, 2018
1 parent 8fd1536 commit bb88602
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 19 deletions.
17 changes: 0 additions & 17 deletions src/main/java/com/conveyal/r5/analyst/NetworkPreloader.java
Original file line number Diff line number Diff line change
Expand Up @@ -84,16 +84,6 @@ public LoaderState<TransportNetwork> preloadData (AnalysisTask task) {
return get(Key.forTask(task));
}

/**
* For regional analysis workers, hackishly bypass our async loading mechanism, relying on the underlying caches.
* That asynchronous loading is mainly useful for single-point tasks where users expect high interactivity.
* This call does not extract the scenario from the task with rememberScenario. That's only necessary in async where
* we key a map on the scenario ID, and in any case we only send full scenarios inside single-point tasks.
*/
public TransportNetwork preloadDataSynchronous (AnalysisTask task) {
return buildValue(Key.forTask(task));
}

@Override
protected TransportNetwork buildValue(Key key) {

Expand Down Expand Up @@ -233,11 +223,4 @@ public PreloadedData(TransportNetwork network, Grid grid, LinkedPointSet linkedP
}
}

/**
* Stopgap measure to load
*/
public TransportNetwork loadNetworkBlocking (String networkId) {
return this.transportNetworkCache.getNetwork(networkId);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,6 @@ protected byte[] handleOneSinglePointTask (TravelTimeSurfaceTask task)
LOG.info("Handling single-point task {}", task.toString());

// Get all the data needed to run one analysis task, or at least begin preparing it.
// TODO synchronously handle regional tasks, or just ensure the specified graph is loaded at worker startup
final AsyncLoader.LoaderState<TransportNetwork> networkLoaderState = networkPreloader.preloadData(task);

// If loading is not complete, bail out of this function.
Expand Down Expand Up @@ -480,7 +479,10 @@ protected void handleOneRegionalTask(RegionalTask task) {
// only be built once.
// Record the currently loaded network ID so we "stick" to this same graph on subsequent polls.
networkId = task.graphId;
TransportNetwork transportNetwork = networkPreloader.preloadDataSynchronous(task);
// Note we're completely bypassing the async loader here and relying on the older nested LoadingCaches.
// If those are ever removed, the async loader will need a synchronous mode with per-key blocking (kind of
// reinventing the wheel of LoadingCache) or we'll need to make preparation for regional tasks async.
TransportNetwork transportNetwork = networkPreloader.transportNetworkCache.getNetwork(task.graphId);

// If we are generating a static site, there must be a single metadata file for an entire batch of results.
// Arbitrarily we create this metadata as part of the first task in the job.
Expand Down

0 comments on commit bb88602

Please sign in to comment.