Update indexes in timetable snapshot #6007

vpaturet · 2024-08-12T19:14:50Z

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 · 2024-08-12T19:30:49Z

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 ⚠️
...pplanner/framework/collection/CollectionUtils.java	33.33%	1 Missing and 1 partial ⚠️
...a/org/opentripplanner/model/TimetableSnapshot.java	97.61%	1 Missing ⚠️

Additional details and impacted files

@@              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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/main/java/org/opentripplanner/model/RealtimeUpdate.java

src/main/java/org/opentripplanner/transit/service/DefaultTransitService.java

habrahamsson-skanetrafiken

This is a great improvement IMO.

Some of my comments are probably out of the scope of this PR and can be done as part of future work.

habrahamsson-skanetrafiken · 2024-08-15T11:36:55Z

src/ext/java/org/opentripplanner/ext/siri/SiriTimetableSnapshotSource.java

+      pattern =
+        tripPatternCache.getOrCreateTripPattern(tripUpdate.stopPattern(), trip, serviceDate);


It's pretty difficult to follow the logic with the different trip patterns being created/fetched depending on if it's an added trip or modified trip. Also this tripPatternCache seems to have a very similar responsibility to the indexes in the snapshot.

But it looks like you are keeping the behaviour the same as previously. So I'm ok with this and we can do any further improvements in a future PR.

I agree, this is quite convoluted and should be addressed in a follow-up PR.

src/main/java/org/opentripplanner/model/TimetableSnapshot.java

habrahamsson-skanetrafiken · 2024-08-15T13:31:52Z

src/main/java/org/opentripplanner/model/TimetableSnapshot.java

+    if (realTimeTripUpdate.routeCreation()) {
+      realtimeAddedRoutes.put(route.getId(), route);
+    }
+    if (realTimeTripUpdate.tripCreation()) {
+      FeedScopedId tripId = trip.getId();
+      realTimeAddedTrips.put(tripId, trip);
+      realTimeAddedPatternForTrip.put(trip, pattern);
+      realTimeAddedPatternForRoute.put(route, pattern);
+      TripOnServiceDate tripOnServiceDate = realTimeTripUpdate.addedTripOnServiceDate();
+      if (tripOnServiceDate != null) {
+        realTimeAddedTripOnServiceDateById.put(tripOnServiceDate.getId(), tripOnServiceDate);
+        realTimeAddedTripOnServiceDateForTripAndDay.put(
+          new TripIdAndServiceDate(tripId, serviceDate),
+          tripOnServiceDate
+        );
+      }


It would be nice with some tests of this functionality. As far as I could see there isn't any

I have added unit tests and fixed some regressions in the process: the TransitService.getAllXXX() methods should also return a combination of scheduled and real-time data.

There is an inconsistency in the existing code with the method TransitService.getAllTripPatterns(): it returns only scheduled trip patterns, not including the ones created in real-time. I keep the existing behavior for now and annotate the method with a TODO.

habrahamsson-skanetrafiken · 2024-08-15T13:49:08Z

src/main/java/org/opentripplanner/model/TimetableSnapshot.java

+      realTimeAddedPatternForTrip.put(trip, pattern);
+      realTimeAddedPatternForRoute.put(route, pattern);


A RealTimeTripUpdate could have a new TripPattern even if it doesn't create a new trip right? As it currently stands these changed patterns won't be added to the index. This is the same as the previous behaviour, so It's not a regression. But we will have to fix this in a follow-up PR if we don't do it now.

True, this should be addressed in a follow-up PR.

vpaturet · 2024-08-20T08:50:26Z

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

vpaturet · 2024-08-20T13:34:36Z

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 9baf38b

habrahamsson-skanetrafiken · 2024-08-21T13:44:57Z

src/main/java/org/opentripplanner/transit/service/TransitService.java

@@ -74,6 +74,9 @@ public interface TransitService {

  Collection<Notice> getNoticesByEntity(AbstractTransitEntity<?, ?> entity);

+  /**
+   * Return a trip pattern by id, not including patterns created by real-time updates.
+   */
  TripPattern getTripPatternForId(FeedScopedId id);


Did you figure out if it is intended that some of these methods that don't include realtime data?

No, the analysis is quite complex. In this PR I will only document the current behavior.

habrahamsson-skanetrafiken · 2024-08-21T13:57:32Z

src/main/java/org/opentripplanner/model/TimetableSnapshot.java

  public Route getRealtimeAddedRoute(FeedScopedId id) {
-    return realtimeAddedRoutes.get(id);
+    return getByNullableKey(id, realtimeAddedRoutes);
  }


I usually prefer to crash when called with an illegal argument, because being lenient in allowing bad input data can make bugs really difficult to find. And i think it's bad practice for callers to assume that downstream code will handle their nulls for them.

But I understand you don't want to go through all the call sites of these methods as part of this PR, so let's leave it for now.

Yes, I try to preserve backward compatibility in this PR, the issue can be addressed in a follow-up PR. The root cause is insufficient data validation in the GraphQL API.

vpaturet · 2024-08-21T15:02:19Z

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 f34b32d

leonardehrenfried · 2024-09-02T09:15:57Z

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

src/main/java/org/opentripplanner/model/TimetableSnapshot.java

t2gran · 2024-09-05T10:11:36Z

src/main/java/org/opentripplanner/model/TimetableSnapshot.java

+    }
+    return map.get(key);
+  }
+


This method can probably be put in the CollectionUtils

leonardehrenfried · 2024-09-06T09:59:29Z

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

# Conflicts: # src/ext-test/java/org/opentripplanner/ext/siri/SiriTimetableSnapshotSourceTest.java

vpaturet added Technical Debt Real-Time Update The issue/PR is related to RealTime updates labels Aug 12, 2024

vpaturet self-assigned this Aug 12, 2024

vpaturet force-pushed the update_indexes_in_timetable_snapshot branch from 4894c6e to 25f6ef3 Compare August 12, 2024 19:18

vpaturet force-pushed the update_indexes_in_timetable_snapshot branch from 25f6ef3 to 70c817b Compare August 13, 2024 08:02

vpaturet marked this pull request as ready for review August 13, 2024 08:03

vpaturet requested a review from a team as a code owner August 13, 2024 08:03

Update indexes in timetable snapshot

5b71ff1

vpaturet force-pushed the update_indexes_in_timetable_snapshot branch from 70c817b to 5b71ff1 Compare August 13, 2024 08:09

habrahamsson-skanetrafiken self-requested a review August 13, 2024 08:28

t2gran added this to the 2.6 (next release) milestone Aug 14, 2024

t2gran requested changes Aug 14, 2024

View reviewed changes

src/main/java/org/opentripplanner/model/RealtimeUpdate.java Outdated Show resolved Hide resolved

src/main/java/org/opentripplanner/model/RealtimeUpdate.java Outdated Show resolved Hide resolved

src/main/java/org/opentripplanner/transit/service/DefaultTransitService.java Show resolved Hide resolved

vpaturet added 2 commits August 15, 2024 11:06

Apply review suggestions

db7ccb4

Rename boolean fields in record

c1c809c

vpaturet requested a review from t2gran August 15, 2024 09:17

habrahamsson-skanetrafiken requested changes Aug 15, 2024

View reviewed changes

vpaturet added 2 commits August 16, 2024 08:45

Apply review suggestion

0caf435

Fix TransitService.getAllXXX methods

1a5c1d5

vpaturet force-pushed the update_indexes_in_timetable_snapshot branch from 79b8ba0 to 1a5c1d5 Compare August 16, 2024 09:50

vpaturet requested a review from habrahamsson-skanetrafiken August 16, 2024 09:56

vpaturet marked this pull request as draft August 20, 2024 08:54

Fix indexing of pattern by stop

e176b0b

vpaturet marked this pull request as ready for review August 20, 2024 10:14

vpaturet added the Entur Test This is currently being tested at Entur label Aug 20, 2024

vpaturet added 2 commits August 20, 2024 14:07

Update GTFS unit tests

49f29b4

Ensure null-safe lookup on immutable map

9baf38b

habrahamsson-skanetrafiken reviewed Aug 21, 2024

View reviewed changes

Use CollectionsView in DefaultTransitService

f34b32d

habrahamsson-skanetrafiken previously approved these changes Aug 21, 2024

View reviewed changes

vpaturet mentioned this pull request Aug 23, 2024

Inject updated TransitService in real-time updaters #6018

Closed

Remove redundant null-check

c0c88ff

vpaturet mentioned this pull request Aug 28, 2024

Introduce RealtimeUpdateContext #6029

Merged

vpaturet dismissed habrahamsson-skanetrafiken’s stale review via c0c88ff August 29, 2024 06:24

t2gran reviewed Sep 5, 2024

View reviewed changes

src/main/java/org/opentripplanner/model/TimetableSnapshot.java Show resolved Hide resolved

t2gran requested changes Sep 5, 2024

View reviewed changes

Apply review suggestions

dae1122

vpaturet requested review from leonardehrenfried, habrahamsson-skanetrafiken and t2gran September 5, 2024 12:32

t2gran previously approved these changes Sep 6, 2024

View reviewed changes

Fix merge conflict

f579cbe

vpaturet dismissed t2gran’s stale review via f579cbe September 10, 2024 18:27

Merge branch 'dev-2.x' into update_indexes_in_timetable_snapshot

c21826b

# Conflicts: # src/ext-test/java/org/opentripplanner/ext/siri/SiriTimetableSnapshotSourceTest.java

vpaturet requested a review from t2gran September 10, 2024 18:41

t2gran approved these changes Sep 11, 2024

View reviewed changes

leonardehrenfried approved these changes Sep 11, 2024

View reviewed changes

vpaturet merged commit 04a9eed into opentripplanner:dev-2.x Sep 11, 2024
5 checks passed

t2gran pushed a commit that referenced this pull request Sep 11, 2024

Add changelog entry for #6007 [ci skip]

f53aa81

t2gran deleted the update_indexes_in_timetable_snapshot branch October 9, 2024 21:12

vpaturet mentioned this pull request Oct 10, 2024

Clear added patterns in TimetableSnapshot #6141

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update indexes in timetable snapshot #6007

Update indexes in timetable snapshot #6007

vpaturet commented Aug 12, 2024 •

edited

Loading

codecov bot commented Aug 12, 2024 •

edited

Loading

habrahamsson-skanetrafiken left a comment

habrahamsson-skanetrafiken Aug 15, 2024

vpaturet Aug 16, 2024

habrahamsson-skanetrafiken Aug 15, 2024

vpaturet Aug 16, 2024 •

edited

Loading

habrahamsson-skanetrafiken Aug 15, 2024

vpaturet Aug 16, 2024

vpaturet commented Aug 20, 2024

vpaturet commented Aug 20, 2024

habrahamsson-skanetrafiken Aug 21, 2024

vpaturet Aug 21, 2024

habrahamsson-skanetrafiken Aug 21, 2024

vpaturet Aug 21, 2024

vpaturet commented Aug 21, 2024

leonardehrenfried commented Sep 2, 2024

t2gran Sep 5, 2024

vpaturet Sep 5, 2024

leonardehrenfried commented Sep 6, 2024

		pattern =
		tripPatternCache.getOrCreateTripPattern(tripUpdate.stopPattern(), trip, serviceDate);

		realTimeAddedPatternForTrip.put(trip, pattern);
		realTimeAddedPatternForRoute.put(route, pattern);

Update indexes in timetable snapshot #6007

Update indexes in timetable snapshot #6007

Conversation

vpaturet commented Aug 12, 2024 • edited Loading

Summary

Issue

Unit tests

Documentation

codecov bot commented Aug 12, 2024 • edited Loading

Codecov Report

habrahamsson-skanetrafiken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpaturet Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpaturet commented Aug 20, 2024

vpaturet commented Aug 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpaturet commented Aug 21, 2024

leonardehrenfried commented Sep 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardehrenfried commented Sep 6, 2024

vpaturet commented Aug 12, 2024 •

edited

Loading

codecov bot commented Aug 12, 2024 •

edited

Loading

vpaturet Aug 16, 2024 •

edited

Loading