From d9310772d92a498a7b937f130020ee16553f7728 Mon Sep 17 00:00:00 2001 From: Michael Tsang Date: Tue, 26 Nov 2024 17:35:47 +0000 Subject: [PATCH 1/4] restore scheduled timetables to TransitLayer when buffer is cleared --- .../org/opentripplanner/model/Timetable.java | 30 ++++ .../model/TimetableSnapshot.java | 34 ++++- .../model/TimetableSnapshotTest.java | 130 +++++++++++++++++- 3 files changed, 192 insertions(+), 2 deletions(-) diff --git a/application/src/main/java/org/opentripplanner/model/Timetable.java b/application/src/main/java/org/opentripplanner/model/Timetable.java index c437a60b0d5..b3bee55e5f7 100644 --- a/application/src/main/java/org/opentripplanner/model/Timetable.java +++ b/application/src/main/java/org/opentripplanner/model/Timetable.java @@ -20,6 +20,7 @@ import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import javax.annotation.Nullable; import org.opentripplanner.transit.model.framework.DataValidationException; @@ -483,4 +484,33 @@ private static TripTimes getRepresentativeTripTimes( return null; } } + + /** + * Get a copy of the scheduled timetable valid for the specified service date only + */ + public Timetable copyForServiceDate(LocalDate date) { + if (serviceDate != null) { + throw new RuntimeException( + "Can only copy scheduled timetable for a specific date if a date hasn't been specified yet." + ); + } + return copyOf().withServiceDate(date).build(); + } + + @Override + public boolean equals(Object o) { + if (o == null || getClass() != o.getClass()) return false; + Timetable timetable = (Timetable) o; + return ( + Objects.equals(pattern, timetable.pattern) && + Objects.equals(tripTimes, timetable.tripTimes) && + Objects.equals(frequencyEntries, timetable.frequencyEntries) && + Objects.equals(serviceDate, timetable.serviceDate) + ); + } + + @Override + public int hashCode() { + return Objects.hash(pattern, tripTimes, frequencyEntries, serviceDate); + } } diff --git a/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java b/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java index 94b490c48a0..9fae2ff5376 100644 --- a/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java +++ b/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java @@ -552,7 +552,39 @@ public boolean isEmpty() { * @return true if the timetable changed as a result of the call */ private boolean clearTimetables(String feedId) { - return timetables.keySet().removeIf(tripPattern -> feedId.equals(tripPattern.getFeedId())); + var dirty = false; + dirtyTimetables.clear(); + + for (var entry : timetables.entrySet()) { + var pattern = entry.getKey(); + + if (feedId.equals(pattern.getFeedId())) { + var timetablesForPattern = entry.getValue(); + var scheduledTimetable = pattern.getScheduledTimetable(); + + // remove scheduled timetables from the entry + var updatedTimetables = timetablesForPattern + .stream() + .filter(timetable -> + !timetable.equals(scheduledTimetable.copyForServiceDate(timetable.getServiceDate())) + ); + + // then restore updated timetables to scheduled timetables + var restoredTimetables = updatedTimetables + .map(timetable -> scheduledTimetable.copyForServiceDate(timetable.getServiceDate())) + .collect(ImmutableSortedSet.toImmutableSortedSet(new SortedTimetableComparator())); + dirty = dirty || !restoredTimetables.isEmpty(); + restoredTimetables.forEach(updated -> + dirtyTimetables.put( + new TripPatternAndServiceDate(pattern, updated.getServiceDate()), + updated + ) + ); + entry.setValue(restoredTimetables); + } + } + + return dirty; } /** diff --git a/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java b/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java index 4f3e12c1368..baf8d930377 100644 --- a/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java +++ b/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java @@ -20,6 +20,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.SortedSet; import java.util.concurrent.atomic.AtomicBoolean; import org.junit.jupiter.api.BeforeAll; @@ -31,7 +32,10 @@ import org.opentripplanner.transit.model.framework.Deduplicator; import org.opentripplanner.transit.model.framework.FeedScopedId; import org.opentripplanner.transit.model.framework.Result; +import org.opentripplanner.transit.model.network.StopPattern; import org.opentripplanner.transit.model.network.TripPattern; +import org.opentripplanner.transit.model.site.RegularStop; +import org.opentripplanner.transit.model.timetable.RealTimeState; import org.opentripplanner.transit.model.timetable.Trip; import org.opentripplanner.transit.model.timetable.TripIdAndServiceDate; import org.opentripplanner.transit.model.timetable.TripOnServiceDate; @@ -46,7 +50,7 @@ public class TimetableSnapshotTest { private static final ZoneId timeZone = ZoneIds.GMT; public static final LocalDate SERVICE_DATE = LocalDate.of(2024, 1, 1); private static Map patternIndex; - static String feedId; + private static String feedId; @BeforeAll public static void setUp() throws Exception { @@ -412,6 +416,130 @@ void testClear() { assertNotNull(snapshot.getRealtimeAddedRoute(pattern.getRoute().getId())); } + /** + * This test checks that the original timetable is given to TransitLayerUpdater for previously + * added patterns after the buffer is cleared. + *

+ * Refer to bug #6197 for details. + */ + @Test + void testTransitLayerUpdateAfterClear() { + var resolver = new TimetableSnapshot(); + + // make an updated trip + var pattern = patternIndex.get(new FeedScopedId(feedId, "1.1")); + var trip = pattern.scheduledTripsAsStream().findFirst().orElseThrow(); + var scheduledTimetable = pattern.getScheduledTimetable(); + var updatedTripTimes = Objects + .requireNonNull(scheduledTimetable.getTripTimes(trip)) + .copyScheduledTimes(); + for (var i = 0; i < updatedTripTimes.getNumStops(); ++i) { + updatedTripTimes.updateArrivalDelay(i, 30); + updatedTripTimes.updateDepartureDelay(i, 30); + } + updatedTripTimes.setRealTimeState(RealTimeState.UPDATED); + var realTimeTripUpdate = new RealTimeTripUpdate( + pattern, + updatedTripTimes, + SERVICE_DATE, + null, + false, + false + ); + + var addedDepartureStopTime = new StopTime(); + var addedArrivalStopTime = new StopTime(); + addedDepartureStopTime.setDepartureTime(0); + addedDepartureStopTime.setArrivalTime(0); + addedDepartureStopTime.setStop(RegularStop.of(new FeedScopedId(feedId, "XX"), () -> 0).build()); + addedArrivalStopTime.setDepartureTime(300); + addedArrivalStopTime.setArrivalTime(300); + addedArrivalStopTime.setStop(RegularStop.of(new FeedScopedId(feedId, "YY"), () -> 1).build()); + var addedStopTimes = List.of(addedDepartureStopTime, addedArrivalStopTime); + var addedStopPattern = new StopPattern(addedStopTimes); + var route = patternIndex.values().stream().findFirst().orElseThrow().getRoute(); + var addedTripPattern = TripPattern + .of(new FeedScopedId(feedId, "1.1")) + .withRoute(route) + .withStopPattern(addedStopPattern) + .withCreatedByRealtimeUpdater(true) + .build(); + var addedTripTimes = TripTimesFactory.tripTimes( + Trip.of(new FeedScopedId(feedId, "addedTrip")).withRoute(route).build(), + addedStopTimes, + new Deduplicator() + ); + var addedTripUpdate = new RealTimeTripUpdate( + addedTripPattern, + addedTripTimes, + SERVICE_DATE, + null, + true, + false + ); + + var transitLayerUpdater = new TransitLayerUpdater(null) { + int count = 0; + + /** + * Test that the TransitLayerUpdater receives correct updated timetables upon commit + *

+ * This method is called 3 times. + * When count = 0, the buffer contains one added and one updated trip, and the timetables + * should reflect this fact. + * When count = 1, the buffer is empty, however, this method should still receive the + * timetables of the previous added and updated patterns in order to restore them to the + * initial scheduled timetable. + * When count = 2, the buffer is still empty, and no changes should be made. + */ + @Override + public void update( + Collection updatedTimetables, + Map> timetables + ) { + assertThat(updatedTimetables).hasSize(count == 2 ? 0 : 2); + + updatedTimetables.forEach(timetable -> { + var timetablePattern = timetable.getPattern(); + assertEquals(SERVICE_DATE, timetable.getServiceDate()); + if (timetablePattern == pattern) { + if (count == 1) { + // the timetable for the existing pattern should revert to the original + assertEquals(scheduledTimetable.getTripTimes(), timetable.getTripTimes()); + } else { + // the timetable for the existing pattern should contain the modified times + assertEquals( + RealTimeState.UPDATED, + Objects.requireNonNull(timetable.getTripTimes(trip)).getRealTimeState() + ); + } + } else if (timetablePattern == addedTripPattern) { + if (count == 1) { + // the timetable for the added pattern should be empty after clearing + assertThat(timetable.getTripTimes()).isEmpty(); + } else { + // the timetable for the added pattern should contain the times for 1 added trip + assertThat(timetable.getTripTimes()).hasSize(1); + } + } else { + throw new RuntimeException("unknown pattern passed to transit layer updater"); + } + }); + ++count; + } + }; + + resolver.update(realTimeTripUpdate); + resolver.update(addedTripUpdate); + resolver.commit(transitLayerUpdater, true); + + resolver.clear(feedId); + resolver.commit(transitLayerUpdater, true); + + resolver.clear(feedId); + resolver.commit(transitLayerUpdater, true); + } + private static TimetableSnapshot createCommittedSnapshot() { TimetableSnapshot timetableSnapshot = new TimetableSnapshot(); return timetableSnapshot.commit(null, true); From 74c86d9a487ef76d4effe6cfdb016833d8e9ede9 Mon Sep 17 00:00:00 2001 From: Michael Tsang Date: Mon, 2 Dec 2024 10:51:45 +0000 Subject: [PATCH 2/4] update test to handle the case when multiple clear calls are invoked between commits --- .../java/org/opentripplanner/model/TimetableSnapshotTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java b/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java index baf8d930377..96e5d562e77 100644 --- a/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java +++ b/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java @@ -533,9 +533,12 @@ public void update( resolver.update(addedTripUpdate); resolver.commit(transitLayerUpdater, true); + resolver.clear(feedId); + resolver.clear(feedId); resolver.clear(feedId); resolver.commit(transitLayerUpdater, true); + resolver.clear(feedId); resolver.clear(feedId); resolver.commit(transitLayerUpdater, true); } From 82a9c295816223cce9ff13ae6348782c371983a6 Mon Sep 17 00:00:00 2001 From: Michael Tsang Date: Mon, 2 Dec 2024 11:11:35 +0000 Subject: [PATCH 3/4] the restored timetables should only be cleared after updating TransitLayer during commit --- .../model/TimetableSnapshot.java | 45 ++++++++++++------- .../model/TimetableSnapshotTest.java | 4 +- 2 files changed, 31 insertions(+), 18 deletions(-) diff --git a/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java b/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java index 9fae2ff5376..a3438eef477 100644 --- a/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java +++ b/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java @@ -96,7 +96,7 @@ public class TimetableSnapshot { * include ones from the scheduled GTFS, as well as ones added by realtime messages and * tracked by the TripPatternCache.

* Note that the keys do not include all scheduled TripPatterns, only those for which we have at - * least one update.

+ * least one update, and those for which we had updates before but just recently cleared.

* The members of the SortedSet (the Timetable for a particular day) are treated as copy-on-write * when we're updating them. If an update will modify the timetable for a particular day, that * timetable is replicated before any modifications are applied to avoid affecting any previous @@ -370,6 +370,32 @@ public TimetableSnapshot commit(TransitLayerUpdater transitLayerUpdater, boolean if (!force && !this.isDirty()) { return null; } + + // update the transitLayer first, including the restored scheduled timetables + if (transitLayerUpdater != null) { + transitLayerUpdater.update(dirtyTimetables.values(), timetables); + } + + this.dirtyTimetables.clear(); + this.dirty = false; + + // remove restored scheduled timetables (restored in clear()) from the snapshot + for (var entry : timetables.entrySet()) { + var pattern = entry.getKey(); + var scheduledTimetable = pattern.getScheduledTimetable(); + entry.setValue( + entry + .getValue() + .stream() + .filter(timetable -> + !timetable.equals(scheduledTimetable.copyForServiceDate(timetable.getServiceDate())) + ) + .collect(ImmutableSortedSet.toImmutableSortedSet(new SortedTimetableComparator())) + ); + } + timetables.entrySet().removeIf(entry -> entry.getValue().isEmpty()); + + // publish the snapshot without the restored scheduled timetables TimetableSnapshot ret = new TimetableSnapshot( Map.copyOf(timetables), Map.copyOf(realTimeNewTripPatternsForModifiedTrips), @@ -383,13 +409,6 @@ public TimetableSnapshot commit(TransitLayerUpdater transitLayerUpdater, boolean true ); - if (transitLayerUpdater != null) { - transitLayerUpdater.update(dirtyTimetables.values(), timetables); - } - - this.dirtyTimetables.clear(); - this.dirty = false; - return ret; } @@ -562,15 +581,9 @@ private boolean clearTimetables(String feedId) { var timetablesForPattern = entry.getValue(); var scheduledTimetable = pattern.getScheduledTimetable(); - // remove scheduled timetables from the entry - var updatedTimetables = timetablesForPattern + // restore updated timetables to scheduled timetables + var restoredTimetables = timetablesForPattern .stream() - .filter(timetable -> - !timetable.equals(scheduledTimetable.copyForServiceDate(timetable.getServiceDate())) - ); - - // then restore updated timetables to scheduled timetables - var restoredTimetables = updatedTimetables .map(timetable -> scheduledTimetable.copyForServiceDate(timetable.getServiceDate())) .collect(ImmutableSortedSet.toImmutableSortedSet(new SortedTimetableComparator())); dirty = dirty || !restoredTimetables.isEmpty(); diff --git a/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java b/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java index 96e5d562e77..cceea964f7d 100644 --- a/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java +++ b/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java @@ -536,11 +536,11 @@ public void update( resolver.clear(feedId); resolver.clear(feedId); resolver.clear(feedId); - resolver.commit(transitLayerUpdater, true); + assertTrue(resolver.commit(transitLayerUpdater, true).isEmpty()); resolver.clear(feedId); resolver.clear(feedId); - resolver.commit(transitLayerUpdater, true); + assertTrue(resolver.commit(transitLayerUpdater, true).isEmpty()); } private static TimetableSnapshot createCommittedSnapshot() { From 00f86bc30aabac1a9115c8770e49c112f0e6aa76 Mon Sep 17 00:00:00 2001 From: Michael Tsang Date: Tue, 17 Dec 2024 18:10:14 +0000 Subject: [PATCH 4/4] no need to put the restored timetables in the snapshot only do the actual restoration when committing if it isn't subsequently made dirty --- .../org/opentripplanner/model/Timetable.java | 17 ---- .../model/TimetableSnapshot.java | 94 ++++++++----------- 2 files changed, 41 insertions(+), 70 deletions(-) diff --git a/application/src/main/java/org/opentripplanner/model/Timetable.java b/application/src/main/java/org/opentripplanner/model/Timetable.java index b3bee55e5f7..edba1ff4b69 100644 --- a/application/src/main/java/org/opentripplanner/model/Timetable.java +++ b/application/src/main/java/org/opentripplanner/model/Timetable.java @@ -496,21 +496,4 @@ public Timetable copyForServiceDate(LocalDate date) { } return copyOf().withServiceDate(date).build(); } - - @Override - public boolean equals(Object o) { - if (o == null || getClass() != o.getClass()) return false; - Timetable timetable = (Timetable) o; - return ( - Objects.equals(pattern, timetable.pattern) && - Objects.equals(tripTimes, timetable.tripTimes) && - Objects.equals(frequencyEntries, timetable.frequencyEntries) && - Objects.equals(serviceDate, timetable.serviceDate) - ); - } - - @Override - public int hashCode() { - return Objects.hash(pattern, tripTimes, frequencyEntries, serviceDate); - } } diff --git a/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java b/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java index 03e8194bae0..11969cf250b 100644 --- a/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java +++ b/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java @@ -14,10 +14,12 @@ import java.util.Comparator; import java.util.ConcurrentModificationException; import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Map.Entry; +import java.util.Set; import java.util.SortedSet; import java.util.TreeSet; import java.util.function.Predicate; @@ -111,6 +113,7 @@ public class TimetableSnapshot { * TripPattern and date. */ private final Map> timetables; + private final Set patternAndServiceDatesToBeRestored = new HashSet<>(); /** * For cases where the trip pattern (sequence of stops visited) has been changed by a realtime @@ -381,32 +384,6 @@ public TimetableSnapshot commit(TransitLayerUpdater transitLayerUpdater, boolean if (!force && !this.isDirty()) { return null; } - - // update the transitLayer first, including the restored scheduled timetables - if (transitLayerUpdater != null) { - transitLayerUpdater.update(dirtyTimetables.values(), timetables); - } - - this.dirtyTimetables.clear(); - this.dirty = false; - - // remove restored scheduled timetables (restored in clear()) from the snapshot - for (var entry : timetables.entrySet()) { - var pattern = entry.getKey(); - var scheduledTimetable = pattern.getScheduledTimetable(); - entry.setValue( - entry - .getValue() - .stream() - .filter(timetable -> - !timetable.equals(scheduledTimetable.copyForServiceDate(timetable.getServiceDate())) - ) - .collect(ImmutableSortedSet.toImmutableSortedSet(new SortedTimetableComparator())) - ); - } - timetables.entrySet().removeIf(entry -> entry.getValue().isEmpty()); - - // publish the snapshot without the restored scheduled timetables TimetableSnapshot ret = new TimetableSnapshot( Map.copyOf(timetables), Map.copyOf(realTimeNewTripPatternsForModifiedTrips), @@ -420,6 +397,25 @@ public TimetableSnapshot commit(TransitLayerUpdater transitLayerUpdater, boolean true ); + if (transitLayerUpdater != null) { + for (var patternAndServiceDate : patternAndServiceDatesToBeRestored) { + if (!dirtyTimetables.containsKey(patternAndServiceDate)) { + var pattern = patternAndServiceDate.tripPattern(); + var scheduledTimetable = pattern.getScheduledTimetable(); + dirtyTimetables.put( + patternAndServiceDate, + scheduledTimetable.copyForServiceDate(patternAndServiceDate.serviceDate) + ); + } + } + + transitLayerUpdater.update(dirtyTimetables.values(), timetables); + } + + patternAndServiceDatesToBeRestored.clear(); + this.dirtyTimetables.clear(); + this.dirty = false; + return ret; } @@ -582,33 +578,25 @@ public boolean isEmpty() { * @return true if the timetable changed as a result of the call */ private boolean clearTimetables(String feedId) { - var dirty = false; - dirtyTimetables.clear(); - - for (var entry : timetables.entrySet()) { - var pattern = entry.getKey(); - - if (feedId.equals(pattern.getFeedId())) { - var timetablesForPattern = entry.getValue(); - var scheduledTimetable = pattern.getScheduledTimetable(); - - // restore updated timetables to scheduled timetables - var restoredTimetables = timetablesForPattern - .stream() - .map(timetable -> scheduledTimetable.copyForServiceDate(timetable.getServiceDate())) - .collect(ImmutableSortedSet.toImmutableSortedSet(new SortedTimetableComparator())); - dirty = dirty || !restoredTimetables.isEmpty(); - restoredTimetables.forEach(updated -> - dirtyTimetables.put( - new TripPatternAndServiceDate(pattern, updated.getServiceDate()), - updated - ) - ); - entry.setValue(restoredTimetables); - } - } - - return dirty; + var entriesToBeRemoved = timetables + .entrySet() + .stream() + .filter(entry -> feedId.equals(entry.getKey().getFeedId())) + .collect(Collectors.toSet()); + patternAndServiceDatesToBeRestored.addAll( + entriesToBeRemoved + .stream() + .flatMap(entry -> + entry + .getValue() + .stream() + .map(timetable -> + new TripPatternAndServiceDate(entry.getKey(), timetable.getServiceDate()) + ) + ) + .toList() + ); + return timetables.entrySet().removeAll(entriesToBeRemoved); } /**