Encapsulate transit model index #5973
Conversation
vpaturet
added
!Technical Debt
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #5973 +/- ##
=============================================
+ Coverage 69.64% 69.66% +0.02%
- Complexity 17139 17165 +26
=============================================
Files 1937 1942 +5
Lines 73747 73784 +37
Branches 7546 7550 +4
=============================================
+ Hits 51362 51403 +41
Misses 19752 19752
+ Partials 2633 2629 -4 ☔ View full report in Codecov by Sentry. |
vpaturet
force-pushed
the
encapsulate_transit_model_index
branch
from
9382541 to
ebb49db
Compare
|
This looks like a good PR. Since it's touching a few tests, would it be possible to wait it until I managed to get #5916 through code review? In fact this PR has had 2 approvals but because of Henrik going away and merge conflicts, it could not be merged yet. We can also decide that it has passed review even though not all requirements were met in the correct order. |
|
I will review your PR, at this stage I guess that is sufficient to get it approved and merged. |
vpaturet
force-pushed
the
encapsulate_transit_model_index
branch
from
ebb49db to
7baf92b
Compare
|
I think treating the index as an implementation detail is a great idea and will make accessing this information a lot more sane. Nevertheless, I think we want to hear @t2gran's take on it. |
abyrd
left a comment
abyrd
left a comment
There was a problem hiding this comment.
The changes look good to me. Your explanation of the current role of each class, and of how this PR moves toward the target design for TransitModel seems consistent. It's noteworthy that TransitService already had most of the necessary methods, with DefaultTransitService reading through to the TransitModelIndex. In many places this PR is just using this existing publicly visible service, allowing the index to become package-private.
t2gran
left a comment
t2gran
left a comment
There was a problem hiding this comment.
Overall this looks ready to merge, but a few JavaDoc comments would be nice.
src/main/java/org/opentripplanner/transit/service/TransitEditorService.java
Show resolved
Hide resolved
src/main/java/org/opentripplanner/updater/vehicle_position/PollingVehiclePositionUpdater.java
Show resolved
Hide resolved
4 participants
Summary
This PR encapsulates the
TransitModelIndexso that it is accessed only through theTransitService/TransitEditorServiceinterfaces.This prepares for further refactoring steps where the mutable state that is currently stored in the
TransitModelIndex, theTimetableSnapshotand theTransitLayerwill be handled in a more consistent and unified way.This also brings the code closer to the target design (immutable transit model) where all transit model updates are performed through the
TransitEditorService.Note:
The
TransitServiceservice should not provide direct read/write access to the underlying collections in the index. Ideally, all accessors should return an unmodifiable view of these collections. This is currently not always the case. This PR fixes this issue in a couple of accessor methods where it is easy to demonstrate that the access is indeed read-only. More accessors should be fixed in a follow-up PR.The content and semantic of
TransitLayer,TimetableSnapshot,TransitModelIndexare not explicit in the current design, but it appears that:TransitLayeris used only in Raptor and reflects all real-time updatesTimetableSnapshotis used in API calls and reflects all real-time updatesTransitModelIndexis used in API calls and reflects scheduled data + initial state of extra journeys(i.e: the only updates to the index are additions of new trips, no removal/modification. Subsequent changes in added trips are not applied to the index)
Issue
No
Unit tests
Updated unit tests
Documentation
No