Refactor graph indexing to allow streetIndex recreation sometimes #12
Conversation
| * intersection, based on input latitude and longitude. Instantiating this class is expensive, | ||
| * because it creates a spatial index of all of the intersections in the graph. | ||
| */ | ||
| public class StreetVertexIndexService { |
There was a problem hiding this comment.
This class probably deserves a comment along the same lines as what was added to Graph#index (i.e., something describing how this was once an interface, but there was no need for the abstraction because of only one type of StreetVertexIndexService ever being used and how this code was copied from X classes).
| @@ -559,7 +558,7 @@ public static void addTransitBidirectional (Graph gg) throws Exception { | |||
| */ | |||
| public static void indexGraphAndLinkStations (Graph g) { | |||
| // creating a new DefaultStreetVertexIndexFactory results in setting the streetIndex on the graph. | |||
| g.index(new DefaultStreetVertexIndexFactory()); | |||
| g.index(false); | |||
There was a problem hiding this comment.
The above comment should probably be changed. Also, it may be obvious to you, but it's somewhat difficult for me to determine in each of these index method calls why we're recreating the street index in some cases but not others.
|
I just added more comments including in all the places where graph.index is called. |
There was a problem hiding this comment.
Looks good. Thanks for those comment changes. Only remaining thought is about the set of log statements that result in building graph over wire:
09:52:48.648 WARN (Graph.java:712) Overwriting an existing streetIndex! This could lead to problems if both the old and new streetIndex are used. Make sure only the new streetIndex is used going forward.
09:52:48.648 INFO (Graph.java:714) building street index
09:52:48.649 WARN (StreetVertexIndexService.java:98) Multiple StreetVertexIndexServices detected on the same graph! Make sure only the most recently created one is used going forward!
09:52:49.056 WARN (StreetSplitter.java:142) Multiple StreetSplitters detected on the same graph! Make sure only the most recently created one is used going forward!
It seems like the first one stating that it is overwriting the streetIndex would make unnecessary the following statements about multiple StreetSplitters detected. Do the older StreetSplitters need to persist after the old streetIndex is overwritten? Maybe I'm misunderstanding the internal steps taken, but instead of saying "Make sure only the most recently created one is used going forward!" couldn't we just overwrite the older StreetSplitter?
|
The issue is that some other class could potentially still be using the old StreetVertexIndexService instance despite it having being overwritten in the graph. Or in the future, someone might decide to write some code that creates a new StreetVertexIndexService or StreetSplitter instance outside of the graph.index method for the same graph. Those log statements are there just in case someone tries to do something funky like that. |
This PR aims to fix #11. After a thorough investigation, it appears that yes, streetIndexes should be recreated, but only in certain cases. Therefore, this PR modifies the
graph.indexmethod to require explicit approval to recreate a streetIndex. Also, it gets rid of some unnecessary classes that aren't needed.Removal of
DefaultStreetVertexIndexFactory.java,StreetVertexIndexFactory.javaand move of code fromStreetVertexIndexServiceImpl.javatoStreetVertexIndexService.javaEach of these classes have sort of been combined into one in order to simplify things.
Discussion about when edges and indexes are created
After researching all possible code paths that use the index data, a few different use cases emerged. Firstly, during graph building, the StreetVertexIndexService is required to be used, but not all modules need the data produced from the indexes in order to work properly. See below for a table detailing how the data is used within each graph builder module.
The second use case involves loading a graph. In this case, the graph has already been built, so a fresh streetIndex is created and will be initialized with all of the data that is needed.
The third problematic use case involves building a graph and then immediately using that graph after it is built. In this case, some vertices and edges created during the StreetLinkerModule and DirectTransferGenerator may not be in every index made available via running of the Graph.index method. Therefore in this case, it does make sense to reindex the graph (including the streetIndex) so all edges are indexed properly. The risk for using 2 StreetSplitters might still exist, however, in practice most future StreetSplits would occur in updaters such as the BikeRentalUpdater which would not use the StreetVertexIndexService created during graph build.