Update indexes in timetable snapshot

vpaturet commented 1 month ago

Summary

As detailed in #5965, updating the transit model indexes directly from the updaters causes data inconsistencies when these indexes are queried concurrently from the API. This PR refactors the real time updaters so that the updating logic is moved from the updaters to the TimetableSnapshot:

The updaters (GTFS and SIRI) produce the entities created in real-time (routes, trip patterns, trips) and forward them to the TimetableSnapShot.
The TimetableSnapshot manages the indexes on these entities and updates them in a thread-safe way (copy-on-write).
The DefaultTransitService queries both the indexes in TimetableSnapshot (real-time data) and the TransitModelIndex (planned data) to provide a unified view of the transit data.

Notes:

The GTFS updater did not initially update all the indexes impacted by real-time updates. This is now the case since the index updates are managed in the TimetableSnapshot.
The exception is the indexing of TripOnServiceDates (DatedServiceJourney) that is supported only in the SIRI updater.

Issue

Close #5965 Close #5665 - The refactoring of the RT code is intended to fix this (this PR and earlier PRs). This is hard to reproduce, reopen if found again.

Unit tests

Updated unit tests.

Documentation

No

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 93.10345% with 10 lines in your changes missing coverage. Please review.

Project coverage is 69.78%. Comparing base (5ba8d94) to head (c21826b). Report is 23 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...planner/transit/service/DefaultTransitService.java	87.03%	7 Missing :warning:
...pplanner/framework/collection/CollectionUtils.java	33.33%	1 Missing and 1 partial :warning:
...a/org/opentripplanner/model/TimetableSnapshot.java	97.61%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## dev-2.x #6007 +/- ## ============================================= + Coverage 69.76% 69.78% +0.02% - Complexity 17327 17356 +29 ============================================= Files 1961 1962 +1 Lines 74267 74359 +92 Branches 7603 7624 +21 ============================================= + Hits 51811 51895 +84 - Misses 19814 19820 +6 - Partials 2642 2644 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

vpaturet commented 1 month ago

I see some missing null-checks in DefaultTransitService, I temporarily switch the PR to draft and fix the code.

vpaturet commented 1 month ago

Fixed a regression caused by the use of immutable maps in TimetableSnapshot: Looking up an immutable map with a null key triggers an NPE:

Exception while fetching data (/serviceJourney) : Cannot invoke "Object.hashCode()" because "pk" is null ```
java.lang.NullPointerException: Cannot invoke "Object.hashCode()" because "pk" is null
    at java.base/java.util.ImmutableCollections$MapN.probe(ImmutableCollections.java:1328)
    at java.base/java.util.ImmutableCollections$MapN.get(ImmutableCollections.java:1242)
    at org.opentripplanner.model.TimetableSnapshot.getRealTimeAddedTrip(TimetableSnapshot.java:238)
    at org.opentripplanner.transit.service.DefaultTransitService.getTripForId(DefaultTransitService.java:284)
    at org.opentripplanner.apis.transmodel.TransmodelGraphQLSchema.lambda$create$36(TransmodelGraphQLSchema.java:1267)
    at graphql.execution.ExecutionStrategy.invokeDataFetcher(ExecutionStrategy.java:533)
    at graphql.execution.ExecutionStrategy.fetchField(ExecutionStrategy.java:497)
    at graphql.execution.ExecutionStrategy.fetchField(ExecutionStrategy.java:438)
    at graphql.execution.ExecutionStrategy.resolveFieldWithInfo(ExecutionStrategy.java:397)
    at graphql.execution.ExecutionStrategy.getAsyncFieldValueInfo(ExecutionStrategy.java:335)
    at graphql.execution.AsyncExecutionStrategy.execute(AsyncExecutionStrategy.java:57)
    at graphql.execution.Execution.executeOperation(Execution.java:180)
    at graphql.execution.Execution.execute(Execution.java:116)
    at graphql.GraphQL.execute(GraphQL.java:546)
    at graphql.GraphQL.lambda$parseValidateAndExecute$13(GraphQL.java:476)
    at java.base/java.util.concurrent.CompletableFuture.uniComposeStage(CompletableFuture.java:1187)
    at java.base/java.util.concurrent.CompletableFuture.thenCompose(CompletableFuture.java:2341)
    at graphql.GraphQL.parseValidateAndExecute(GraphQL.java:471)
    at graphql.GraphQL.lambda$executeAsync$9(GraphQL.java:429)
    at java.base/java.util.concurrent.CompletableFuture.uniComposeStage(CompletableFuture.java:1187)
    at java.base/java.util.concurrent.CompletableFuture.thenCompose(CompletableFuture.java:2341)
    at graphql.GraphQL.executeAsync(GraphQL.java:418)
    at graphql.GraphQL.execute(GraphQL.java:359)

This is fixed in https://github.com/opentripplanner/OpenTripPlanner/pull/6007/commits/9baf38b001af08c6ee709d2ae84f660b9bbfc268

vpaturet commented 1 month ago

Merging HashSets from scheduled data and real-time data is suboptimal. There are cases where it is easy to prove that the data do not overlap. In this case we can use a CollectionsView. Fixed in https://github.com/opentripplanner/OpenTripPlanner/pull/6007/commits/f34b32dcaf89bfb8db984a6c70b46706862b607a

leonardehrenfried commented 1 month ago

I really like where these PRs are going, namely by introducing some well-needed structure into the storage of the real-time data.

leonardehrenfried commented 4 weeks ago

I don't think I have been formally assigned the reviewer. Should I review or is it @habrahamsson-skanetrafiken ?

opentripplanner / OpenTripPlanner