From f91086e069907b708d5818037dfca23908820757 Mon Sep 17 00:00:00 2001 From: Vincent Paturet Date: Sun, 13 Oct 2024 18:04:08 +0200 Subject: [PATCH 1/3] Fix duplicated dirty timetables --- .../model/TimetableSnapshot.java | 26 ++++++++++++++----- .../transit/mappers/TransitLayerUpdater.java | 2 +- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java b/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java index 6b1190a66b3..8dd87a27e22 100644 --- a/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java +++ b/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java @@ -13,11 +13,9 @@ import java.util.Comparator; import java.util.ConcurrentModificationException; import java.util.HashMap; -import java.util.HashSet; import java.util.Iterator; import java.util.Map; import java.util.Map.Entry; -import java.util.Set; import java.util.SortedSet; import java.util.TreeSet; import javax.annotation.Nullable; @@ -83,10 +81,14 @@ public class TimetableSnapshot { /** * During the construction phase of the TimetableSnapshot, before it is considered immutable and - * used in routing, this Set holds all timetables that have been modified and are waiting to be - * indexed. This field will be set to null when the TimetableSnapshot becomes read-only. + * used in routing, this Map holds all timetables that have been modified and are waiting to be + * indexed. + * A real-time timetable overrides the scheduled timetable of a TripPattern for only a single + * service date. There can be only one overriding timetable per TripPattern and per service date. + * This is enforced by indexing the map with a pair (TripPattern, service date). + * This field will be set to null when the TimetableSnapshot becomes read-only. */ - private final Set dirtyTimetables = new HashSet<>(); + private final Map dirtyTimetables = new HashMap<>(); /** * For each TripPattern (sequence of stops on a particular Route) for which we have received a @@ -383,7 +385,7 @@ public TimetableSnapshot commit(TransitLayerUpdater transitLayerUpdater, boolean ); if (transitLayerUpdater != null) { - transitLayerUpdater.update(dirtyTimetables, timetables); + transitLayerUpdater.update(dirtyTimetables.values(), timetables); } this.dirtyTimetables.clear(); @@ -600,7 +602,12 @@ private void swapTimetable(TripPattern pattern, Timetable original, Timetable up } sortedTimetables.add(updated); timetables.put(pattern, ImmutableSortedSet.copyOfSorted(sortedTimetables)); - dirtyTimetables.add(updated); + + // if the timetable was already modified by a previous real-time update in the same snapshot + // and for the same service date, + // then the previously updated timetable is superseded by the new one + dirtyTimetables.put(new TripPatternAndServiceDate(pattern, updated.getServiceDate()), updated); + dirty = true; } @@ -617,4 +624,9 @@ public int compare(Timetable t1, Timetable t2) { return t1.getServiceDate().compareTo(t2.getServiceDate()); } } + + /** + * A pair made of a TripPattern and one of the service dates it is running on. + */ + private record TripPatternAndServiceDate(TripPattern tripPattern, LocalDate serviceDate) {} } diff --git a/application/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/mappers/TransitLayerUpdater.java b/application/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/mappers/TransitLayerUpdater.java index 934bec39c11..d3e6cd07d60 100644 --- a/application/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/mappers/TransitLayerUpdater.java +++ b/application/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/transit/mappers/TransitLayerUpdater.java @@ -60,7 +60,7 @@ public TransitLayerUpdater(TransitEditorService transitService) { } public void update( - Set updatedTimetables, + Collection updatedTimetables, Map> timetables ) { if (!transitService.hasRealtimeTransitLayer()) { From a12913110b26b4de92cf6d195a4307948e21ccbe Mon Sep 17 00:00:00 2001 From: Vincent Paturet Date: Sun, 13 Oct 2024 19:43:58 +0200 Subject: [PATCH 2/3] Added unit test --- .../model/TimetableSnapshotTest.java | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java b/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java index ffdfc027b76..b6693bf0a54 100644 --- a/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java +++ b/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java @@ -14,18 +14,28 @@ import com.google.transit.realtime.GtfsRealtime.TripUpdate; import java.time.LocalDate; import java.time.ZoneId; +import java.util.Collection; import java.util.ConcurrentModificationException; import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.SortedSet; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.opentripplanner.ConstantsForTests; import org.opentripplanner.TestOtpModel; import org.opentripplanner._support.time.ZoneIds; +import org.opentripplanner.routing.algorithm.raptoradapter.transit.TransitLayer; +import org.opentripplanner.routing.algorithm.raptoradapter.transit.mappers.TransitLayerUpdater; +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.TripPattern; +import org.opentripplanner.transit.model.timetable.Trip; +import org.opentripplanner.transit.model.timetable.TripIdAndServiceDate; +import org.opentripplanner.transit.model.timetable.TripOnServiceDate; import org.opentripplanner.transit.model.timetable.TripTimes; +import org.opentripplanner.transit.model.timetable.TripTimesFactory; import org.opentripplanner.transit.service.TransitModel; import org.opentripplanner.updater.spi.UpdateError; import org.opentripplanner.updater.trip.BackwardsDelayPropagationType; @@ -33,6 +43,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; @@ -261,6 +272,44 @@ public void testPurge() { assertFalse(resolver.isDirty()); } + @Test + void testUniqueDirtyTimetablesAfterMultipleUpdates() { + TimetableSnapshot snapshot = new TimetableSnapshot(); + TripPattern pattern = patternIndex.get(new FeedScopedId(feedId, "1.1")); + Trip trip = pattern.scheduledTripsAsStream().findFirst().orElseThrow(); + + TripTimes updatedTriptimes = TripTimesFactory.tripTimes( + trip, + List.of(new StopTime()), + new Deduplicator() + ); + RealTimeTripUpdate realTimeTripUpdate = new RealTimeTripUpdate( + pattern, + updatedTriptimes, + SERVICE_DATE, + TripOnServiceDate.of(trip.getId()).withTrip(trip).withServiceDate(SERVICE_DATE).build(), + true, + true + ); + + snapshot.update(realTimeTripUpdate); + snapshot.update(realTimeTripUpdate); + assertTrue(snapshot.isDirty()); + + TransitLayerUpdater transitLayer = new TransitLayerUpdater(null) { + @Override + public void update( + Collection updatedTimetables, + Map> timetables + ) { + assertEquals(1, updatedTimetables.size()); + assertEquals(1, timetables.size()); + } + }; + + snapshot.commit(transitLayer, true); + } + @Test void testCannotUpdateReadOnlyTimetableSnapshot() { TimetableSnapshot committedSnapshot = createCommittedSnapshot(); From 7644483fc2cba1dc308077ee785f5fdb1b59c2e2 Mon Sep 17 00:00:00 2001 From: Vincent Paturet Date: Mon, 14 Oct 2024 08:45:15 +0200 Subject: [PATCH 3/3] Apply review suggestions --- .../opentripplanner/model/TimetableSnapshot.java | 2 +- .../model/TimetableSnapshotTest.java | 13 +++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java b/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java index 8dd87a27e22..bb02695122e 100644 --- a/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java +++ b/application/src/main/java/org/opentripplanner/model/TimetableSnapshot.java @@ -86,7 +86,7 @@ public class TimetableSnapshot { * A real-time timetable overrides the scheduled timetable of a TripPattern for only a single * service date. There can be only one overriding timetable per TripPattern and per service date. * This is enforced by indexing the map with a pair (TripPattern, service date). - * This field will be set to null when the TimetableSnapshot becomes read-only. + * This map is cleared when the TimetableSnapshot becomes read-only. */ private final Map dirtyTimetables = new HashMap<>(); diff --git a/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java b/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java index b6693bf0a54..89766c0aca5 100644 --- a/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java +++ b/application/src/test/java/org/opentripplanner/model/TimetableSnapshotTest.java @@ -1,5 +1,6 @@ package org.opentripplanner.model; +import static com.google.common.truth.Truth.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; @@ -20,19 +21,18 @@ import java.util.List; import java.util.Map; import java.util.SortedSet; +import java.util.concurrent.atomic.AtomicBoolean; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.opentripplanner.ConstantsForTests; import org.opentripplanner.TestOtpModel; import org.opentripplanner._support.time.ZoneIds; -import org.opentripplanner.routing.algorithm.raptoradapter.transit.TransitLayer; import org.opentripplanner.routing.algorithm.raptoradapter.transit.mappers.TransitLayerUpdater; 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.TripPattern; import org.opentripplanner.transit.model.timetable.Trip; -import org.opentripplanner.transit.model.timetable.TripIdAndServiceDate; import org.opentripplanner.transit.model.timetable.TripOnServiceDate; import org.opentripplanner.transit.model.timetable.TripTimes; import org.opentripplanner.transit.model.timetable.TripTimesFactory; @@ -296,18 +296,23 @@ void testUniqueDirtyTimetablesAfterMultipleUpdates() { snapshot.update(realTimeTripUpdate); assertTrue(snapshot.isDirty()); + AtomicBoolean updateIsCalled = new AtomicBoolean(); + TransitLayerUpdater transitLayer = new TransitLayerUpdater(null) { @Override public void update( Collection updatedTimetables, Map> timetables ) { - assertEquals(1, updatedTimetables.size()); - assertEquals(1, timetables.size()); + updateIsCalled.set(true); + assertThat(updatedTimetables).hasSize(1); + assertThat(timetables).hasSize(1); } }; snapshot.commit(transitLayer, true); + + assertTrue(updateIsCalled.get()); } @Test