Skip to content

Commit

Permalink
Add more Javadoc to clarify splitting types
Browse files Browse the repository at this point in the history
  • Loading branch information
@evansiroky
evansiroky committed May 11, 2019
1 parent cd8b240 commit cf74d81
Showing 1 changed file with 15 additions and 3 deletions.
18 changes: 15 additions & 3 deletions src/main/java/org/opentripplanner/graph_builder/linking/StreetSplitter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,19 @@ public class StreetSplitter {

public static final int WARNING_DISTANCE_METERS = 20;

// DESTRUCTIVE_SPLIT means edges are split and new edges are created (used when linking transit stops etc. during graph building)
// NON_DESTRUCTIVE_SPLIT means new temporary edges are created and no edges are deleted (Used when searching for origin/destination)
/**
* DESTRUCTIVE_SPLIT means edges are split and new edges and vertices are created. This should occur when linking
* transit stops etc. during graph building or inserting floating bicycles in a bikeshare system in realtime into
* the graph. When these splits occur, a spatial index of all edges should be kept updated, otherwise there will

This comment has been minimized.

Sign in to view

Copy link
@abyrd

abyrd May 22, 2019

Member

The destructive splitting operation was originally only intended for use during build or startup operations, with the street graph considered read-only thereafter. If it's being used while the server is up and receiving requests, I don't think thre's any guarantee it's threadsafe and will work properly (hence the potential problems with spatial indexes being written to in a non-threadsafe way).

I wouldn't say "there will appear to be snapping issues" because it's not a matter of appearance, there will in fact be problems. I also think the term "snapping" is ill-defined and might not be understood by someone approaching the code for the first time. I think we should give a deeper explanation of why a spatial index is maintained from a design perspective: some edges are temporary (request-scoped, used by only one thread, and never seen and re-split by other threads) while other edges are long-lived, permanent or semi-permanent parts of a data structure shared across threads, and the latter should be seen and re-split by threads other than the one creating them.
* appear to be snapping issues as observed in https://github.com/opentripplanner/OpenTripPlanner/issues/2758.
*/
public static final boolean DESTRUCTIVE_SPLIT = true;

/**
* NON_DESTRUCTIVE_SPLIT means new temporary edges and vertices are created and no edges are deleted. This is used
* in individual requests when temporarily inserting the origin/destination into the graph. The spatial index of
* street edges should not be updated to include these temporary edges.

This comment has been minimized.

Sign in to view

Copy link
@abyrd

abyrd May 22, 2019

Member

We should probably elaborate on why the spatial index is not updated. The reason is that we don't want other requests in other threads finding and re-splitting edges that are only relevant (and intended to be visible) to a single request.
*/
public static final boolean NON_DESTRUCTIVE_SPLIT = false;

/** if there are two ways and the distances to them differ by less than this value, we link to both of them */
Expand Down Expand Up @@ -188,7 +198,9 @@ public boolean linkToGraph(Vertex vertex, TraverseMode traverseMode, RoutingRequ
}

/**
* Link this vertex into the graph
* Link this vertex into the graph (ie, connect the vertex to other vertices in the graph by creating one or more
* new edges/vertexes which are then connected to the other vertices/edges of the graph.
*
* @param vertex The vertex to be linked.
* @param traverseModeSet The traverse modes.
* @param options The routing options.
Expand Down

0 comments on commit cf74d81

Please sign in to comment.