Remove circular dependency between graph and vertex linker #6686
Conversation
leonardehrenfried
added
Skip Changelog
bump serialization id
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #6686 +/- ##
==========================================
Coverage 71.57% 71.58%
- Complexity 18930 18934 +4
==========================================
Files 2059 2061 +2
Lines 77467 77472 +5
Branches 7900 7902 +2
==========================================
+ Hits 55449 55456 +7
- Misses 19200 19202 +2
+ Partials 2818 2814 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
leonardehrenfried
force-pushed
the
vertex-linker
branch
2 times, most recently
from
9507b98 to
f2a9eff
Compare
|
@vesameskanen You might be interested in this. |
leonardehrenfried
force-pushed
the
vertex-linker
branch
from
7ad36df to
afe4d6e
Compare
leonardehrenfried
force-pushed
the
vertex-linker
branch
from
afe4d6e to
c7ea41e
Compare
|
There are conflicts |
There was a problem hiding this comment.
Very good changes. My only remark was about graph indexing - which of the two index methods should a new builder module use? Maybe builder module execution order dictates that, but such a logic is hidden in the code.
Also, I see merge conflicts, please fix them.
| index(); | ||
| } | ||
| } | ||
|
|
||
| @Nullable |
There was a problem hiding this comment.
Difference between index and requestIndex feels a bit vague. Maybe some more/updated documentation? At least the index method docs seem to be outdated.
There was a problem hiding this comment.
The strategy for indexing during graph build is as follows:
-
avoid indexing, this can be programaticly done by using builders and immutable repositories.
-
mappers in need of an index should create their own - this is in most cases cheeper than reindexing everything every time a mapper needs an index.
-
for real-time updated or request indexes a case-to-case strategy is needed.
I have not looked at the use-cases for the graph indexing, so I do not know what is the right strategy for it. But, leaving indexing to the user (calls like graph.index()) is domed to fail. Encapsulating the indexing (dirty flag strategy) is a little better, but it is also problematic since you risk reindexing a lot - the caller think he is safe - but if he uses the wrong combination of methods he might cause a lot of reindexing - without a clue before running the code.
# Conflicts: # application/src/main/java/org/opentripplanner/standalone/server/DefaultServerRequestContext.java # Conflicts: # application/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/router/street/DirectStreetRouter.java # application/src/main/java/org/opentripplanner/routing/graphfinder/GraphFinder.java # application/src/main/java/org/opentripplanner/routing/graphfinder/StreetGraphFinder.java # application/src/main/java/org/opentripplanner/standalone/api/OtpServerRequestContext.java # application/src/main/java/org/opentripplanner/visualizer/GraphVisualizer.java
leonardehrenfried
force-pushed
the
vertex-linker
branch
from
c7ea41e to
b56591a
Compare
| public enum VisibilityMode { | ||
| COMPUTE_AREA_VISIBILITY, | ||
| SKIP_AREA_VISIBILITY; | ||
|
|
||
| public static VisibilityMode ofBoolean(boolean computeVisibility) { | ||
| if (computeVisibility) { | ||
| return COMPUTE_AREA_VISIBILITY; | ||
| } else { | ||
| return SKIP_AREA_VISIBILITY; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Public classes (enums are classes) should be defined in their own file, not as inner classes - only private classes can be defined inside a class.
| COMPUTE_AREA_VISIBILITY, | ||
| SKIP_AREA_VISIBILITY; |
There was a problem hiding this comment.
| COMPUTE_AREA_VISIBILITY, | |
| SKIP_AREA_VISIBILITY; | |
| COMPUTE_AREA_VISIBILITY_LINES, | |
| TRAVERS_AREA_EDGES; |
Also this need JavaDoc
| index(); | ||
| } | ||
| } | ||
|
|
||
| @Nullable |
There was a problem hiding this comment.
The strategy for indexing during graph build is as follows:
-
avoid indexing, this can be programaticly done by using builders and immutable repositories.
-
mappers in need of an index should create their own - this is in most cases cheeper than reindexing everything every time a mapper needs an index.
-
for real-time updated or request indexes a case-to-case strategy is needed.
I have not looked at the use-cases for the graph indexing, so I do not know what is the right strategy for it. But, leaving indexing to the user (calls like graph.index()) is domed to fail. Encapsulating the indexing (dirty flag strategy) is a little better, but it is also problematic since you risk reindexing a lot - the caller think he is safe - but if he uses the wrong combination of methods he might cause a lot of reindexing - without a clue before running the code.
Summary
As a follow up of #6605 I'm removing the circular dependency between graph and vertex linker.
Graphs now don't know anything about linking anymore and simply are repositories for street data.
The instantiation of the vertex linker is now managed by Dagger and there are two modules to do it:
The vertex linker is now immutable so you don't need to take a lot of care to make sure that you only have a single instance. Nevertheless, Dagger makes sure.
To get an idea of the new relationships check the updated diagram.
Unit tests
Lots updated.
Documentation
Javadoc.
Bumping the serialization version id
Yes, Graph is changed.