Fix real-time added patterns persistence with DIFFERENTIAL updates by optionsome · Pull Request #5726 · opentripplanner/OpenTripPlanner

optionsome · 2024-03-04T21:09:13Z

Summary

There is an issue with "cancelling" a stop closure when using DIFFERENTIAL GTFS realtime trip updates. A realtime added trip pattern for the trip remains in a broken state after a follow up update to the trip.

This also contains minor refactoring.

Issue

Closes #5725

Unit tests

Added tests

Documentation

Not needed

Changelog

From title

…pdates

codecov · 2024-03-04T21:15:10Z

Codecov Report

Attention: Patch coverage is 77.19298% with 13 lines in your changes missing coverage. Please review.

Project coverage is 68.64%. Comparing base (06a7e8c) to head (bc06c14).
Report is 97 commits behind head on dev-2.x.

Files	Patch %	Lines
...a/org/opentripplanner/model/TimetableSnapshot.java	66.66%	2 Missing and 6 partials ⚠️
...pplanner/updater/trip/TimetableSnapshotSource.java	84.37%	2 Missing and 3 partials ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5726      +/-   ##
=============================================
+ Coverage      68.54%   68.64%   +0.10%     
- Complexity     16732    16838     +106     
=============================================
  Files           1918     1927       +9     
  Lines          72734    72943     +209     
  Branches        7456     7481      +25     
=============================================
+ Hits           49854    50071     +217     
+ Misses         20310    20292      -18     
- Partials        2570     2580      +10

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

t2gran · 2024-03-08T23:34:20Z

@optionsome What is the effective change for the SiriTimetableSnapshotSource? Is the code just pushed "down" or is there actual changes. It is a bit hard to compare it.

leonardehrenfried · 2024-03-11T12:29:32Z

I'm afraid that this logic is too brittle to go without unit tests documenting the use cases. I think we need to refactor to make unit tests possible.

leonardehrenfried · 2024-03-11T12:38:20Z

-    // If this trip_id has been used for previously ADDED/MODIFIED trip message (e.g. when the
-    // sequence of stops has changed, and is now changing back to the originally scheduled one),
-    // mark that previously created trip as DELETED.
-    cancelPreviouslyAddedTrip(tripId, serviceDate, CancelationType.DELETE);


Are you moving these call up because they happen for every scenario or is there another reason for this?

There were two reasons, one of them was that I saw this was used in every scenario and the other was that I was unsure what would happen if I run the new code before this. However, I think moving this creates a minor regression as now the previously added trip is always deleted. However, I think previously if you sent a CANCELLED message on the trip, it was just cancelled, not removed. Do we want to keep that behavior?

So an ADDED trip was then CANCELLED again? If so, I'm quite relaxed about that but I would like to see tests documenting all these weird edge cases.

If it's just cancelling ADDDED trips, I think it doesn't matter.

@t2gran do you know how cancellation of an added trip works in SIRI? Do we cancel or remove it?

I think we should cancel it. I have asked @lassetyr about this now, and in the future we want to be able to "cancel" any previously added update - DELETE in GTFS?

Summary of my thought about this - related to todays dev meeting discussion:

Trip updates is applied to the PLANED trips

not the previous updated trip(if it exist)

if the trip do not exist the update must create a new trip and set the appropriate flags. This mean in the future that we might need to support incomplete trips (trip update do not have a complete set of data).

To support scalability beyond the current demand, we might have to support incremental updates(incomplete trip data). If done, it needs to be combined with a "aggregator-service" witch keep track of the complete state and can serve the full-updates to OTP instance starting up, or resetting the RT state (periodically). For reference, look at the event-sourcing design pattern.

optionsome · 2024-03-11T12:53:08Z

@optionsome What is the effective change for the SiriTimetableSnapshotSource? Is the code just pushed "down" or is there actual changes. It is a bit hard to compare it.

Currently the code is just pushed down. However, I'm still slightly evaluating what to do with realtime added trips so I might have to split some functionality.

optionsome · 2024-03-14T09:50:17Z

@optionsome What is the effective change for the SiriTimetableSnapshotSource? Is the code just pushed "down" or is there actual changes. It is a bit hard to compare it.

Currently the code is just pushed down. However, I'm still slightly evaluating what to do with realtime added trips so I might have to split some functionality.

Didn't end up refactoring this more so the code is just pushed to the TimetableSnapshot, nothing really at least should have changed for the SIRI updater.

leonardehrenfried · 2024-03-15T06:24:20Z

+              case CANCELED -> handleCanceledTrip(
+                tripId,
+                serviceDate,
+                CancelationType.CANCEL,
+                canceledPreviouslyAddedTrip
+              );
+              case DELETED -> handleCanceledTrip(
+                tripId,
+                serviceDate,
+                CancelationType.DELETE,
+                canceledPreviouslyAddedTrip
+              );


I'm not too happy about passing down a boolean to method which leads to an immediate return and I would like to handle it earlier.

In an ideal world Java would have a syntax like this:

case CANCELED if canceledPreviouslyAddedTrip -> Result.success(UpdateSuccess.noWarnings());

but it doesn't.

How about catching that case before the switch? So something like

if (CANCELLED or DELETED) and canceledPreviouslyAddedTrip return Result.success(UpdateSuccess.noWarnings());

In general I agree with you. However, the handleCanceledTrip is where you expect the handling of any valid case of a CANCELED/DELETED scheduled relationship to be.

optionsome · 2024-06-03T18:09:15Z

I'm sorry, don't review this even though I requested review. I realized I've by mistake included a commit which I forgot to remove a week ago which explains a lot of confusion I've had today related to this pr... I'll try to properly fix this pr tomorrow hopefully.

optionsome · 2024-06-04T07:59:37Z

I've hopefully now fixed the problems. I unfortunately had to force push due to accidentally leaving in a commit I shouldn't have previously.

optionsome · 2024-06-04T08:10:40Z

-   * @param serviceDate service date
   * @return true if a previously added trip was cancelled
   */
  private boolean cancelPreviouslyAddedTrip(


The naming of this method is slightly misleading.

I've limited what this method does so the naming is more correct now. I think this method was now otherwise doing duplicate stuff with the new clean-up method I've introduced to the GTFS RT logic.

I now realize that 1cb32d9 commit title is unreadable but I don't think I'm going to fix it as we usually don't fix typos in commit titles 😅

optionsome · 2024-06-04T15:50:09Z

@leonardehrenfried @abyrd this pr is now ready for review again.

leonardehrenfried · 2024-06-04T16:04:41Z

+    return applyTripUpdates(List.of(update), false);
+  }
+
+  public UpdateResult applyTripUpdate(GtfsRealtime.TripUpdate update, boolean differential) {


Can you convert the boolean to an enum making it clear what exactly that means? After some discussion in another PR we concluded that we want to change to an enum in the main code as well. That I will do in a separate PR.

To avoid conflicts, maybe it's easier if this gets either updated in this pr or in your pr depending on what gets merged first.

Yes, lets do that.

leonardehrenfried · 2024-06-05T05:53:30Z

+        if (!fullDataset) {
+          // Check whether trip id has been used for previously ADDED trip message and mark previously
+          // created trip as DELETED unless schedule relationship is CANCELED, then as CANCEL
+          var cancelationType = tripScheduleRelationship ==
+            TripDescriptor.ScheduleRelationship.CANCELED
+            ? CancelationType.CANCEL
+            : CancelationType.DELETE;
+          canceledPreviouslyAddedTrip =
+            cancelPreviouslyAddedTrip(tripId, serviceDate, cancelationType);
+          // Remove previous realtime updates for this trip. This is necessary to avoid previous
+          // stop pattern modifications from persisting. If a trip was previously added with the ScheduleRelationship
+          // ADDED and is now cancelled or deleted, we still want to keep the realtime added trip pattern.
+          if (
+            !canceledPreviouslyAddedTrip ||
+            (
+              tripScheduleRelationship != TripDescriptor.ScheduleRelationship.CANCELED &&
+              tripScheduleRelationship != TripDescriptor.ScheduleRelationship.DELETED
+            )
+          ) {
+            this.buffer.revertTripToScheduledTripPattern(tripId, serviceDate);
+          }
+        }


Can you move this into a method, please?

Actually, why don't you do all of this logic in handelCancelledTrip?

I just don't want to see half the logic on one level of abstraction and the other half in another one.

optionsome · 2024-06-07T09:26:46Z

This is now ready for review again. I realized there was a potential issue with determining if trip was added based on the original trip pattern because of the trip pattern cache so I changed the logic to check the realtime state instead. I didn't update #5726 (comment) yet as there can be potential naming issues with the enum and I don't want accidentally end up with duplicate enum.

leonardehrenfried · 2024-06-07T10:04:06Z

+  private void purgePatternModifications(
+    TripDescriptor.ScheduleRelationship tripScheduleRelationship,
+    FeedScopedId tripId,
+    LocalDate serviceDate
+  ) {
+    final TripPattern pattern = buffer.getRealtimeAddedTripPattern(tripId, serviceDate);
+    if (
+      !isPreviouslyAddedTrip(tripId, pattern, serviceDate) ||
+      (
+        tripScheduleRelationship != TripDescriptor.ScheduleRelationship.CANCELED &&
+        tripScheduleRelationship != TripDescriptor.ScheduleRelationship.DELETED
+      )
+    ) {
+      // Remove previous realtime updates for this trip. This is necessary to avoid previous
+      // stop pattern modifications from persisting. If a trip was previously added with the ScheduleRelationship
+      // ADDED and is now cancelled or deleted, we still want to keep the realtime added trip pattern.
+      this.buffer.revertTripToScheduledTripPattern(tripId, serviceDate);
+    }
+  }
+
+  private boolean isPreviouslyAddedTrip(
+    FeedScopedId tripId,
+    TripPattern pattern,
+    LocalDate serviceDate
+  ) {
+    if (pattern == null) {
+      return false;
+    }
+    var timetable = buffer.resolve(pattern, serviceDate);
+    if (timetable == null) {
+      return false;
+    }
+    var tripTimes = timetable.getTripTimes(tripId);
+    if (tripTimes == null) {
+      return false;
+    }
+    return tripTimes.getRealTimeState() == RealTimeState.ADDED;
+  }
+


This is much nicer!

Fix issue with realtime added patterns persisting with DIFFERENTIAL u…

e432a38

…pdates

optionsome added !Bug Apply to issues describing a bug and PRs witch fixes it. +Real-Time The issue/PR is related to RealTime updates labels Mar 4, 2024

optionsome added this to the 2.5 (next release) milestone Mar 4, 2024

optionsome requested a review from a team as a code owner March 4, 2024 21:09

leonardehrenfried self-requested a review March 5, 2024 10:34

Only run removal of old leftovers if DIFFERENTIAL update

d3f467d

leonardehrenfried reviewed Mar 11, 2024

View reviewed changes

optionsome added 2 commits March 12, 2024 17:20

Set added trip as canceled again if canceled update

9630a4a

Add test for removing skipped update

0177fb4

t2gran modified the milestones: 2.5, 2.6 (next release) Mar 13, 2024

Add tests for canceling/deleting added trips

f518363

optionsome requested review from leonardehrenfried and t2gran March 14, 2024 09:49

Fix formatting

dfe08af

leonardehrenfried reviewed Mar 14, 2024

View reviewed changes

Comment thread src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java Outdated

Use parametrized test

7cdbe3d

leonardehrenfried changed the title ~~Fix an issue with realtime added patterns persisting with DIFFERENTIAL updates~~ Fixrealtime added patterns persistence with DIFFERENTIAL updates Mar 14, 2024

leonardehrenfried changed the title ~~Fixrealtime added patterns persistence with DIFFERENTIAL updates~~ Fix real-time added patterns persistence with DIFFERENTIAL updates Mar 14, 2024

leonardehrenfried requested review from abyrd and leonardehrenfried and removed request for t2gran March 14, 2024 14:27

optionsome added the Digitransit Test Feature is under testing in Digitransit environment(s) label Mar 14, 2024

leonardehrenfried reviewed Mar 15, 2024

View reviewed changes

optionsome requested a review from leonardehrenfried June 3, 2024 17:07

optionsome marked this pull request as draft June 3, 2024 18:09

optionsome added 3 commits June 4, 2024 10:12

Merge remote-tracking branch 'upstream/dev-2.x' into fix-skipped-removal

fe8d52b

Fix added trip cancellation/deletion logic

b9526c3

Remove some unnecessary param javadoc

bdf7161

optionsome force-pushed the fix-skipped-removal branch from ce0eeaa to bdf7161 Compare June 4, 2024 07:58

optionsome marked this pull request as ready for review June 4, 2024 07:58

optionsome commented Jun 4, 2024

View reviewed changes

optionsome added 3 commits June 4, 2024 11:59

Clarify javadoc and only remove added removal when trip was added

1cb32d9

Merge remote-tracking branch 'upstream/dev-2.x' into fix-skipped-removal

51d7550

Refactor new tests to be module tests

441bd5c

leonardehrenfried reviewed Jun 4, 2024

View reviewed changes

Comment thread src/test/java/org/opentripplanner/updater/trip/TimetableSnapshotSourceTest.java Outdated

leonardehrenfried reviewed Jun 5, 2024

View reviewed changes

Comment thread src/test/java/org/opentripplanner/updater/trip/RealtimeTestEnvironment.java Outdated

leonardehrenfried reviewed Jun 5, 2024

View reviewed changes

leonardehrenfried mentioned this pull request Jun 5, 2024

Extract timetable manager for realtime trip updates #5882

Merged

optionsome added 2 commits June 7, 2024 11:09

Clean up tests

83da1d8

Refactor and fix potential issue with added trip not being detected

bc06c14

leonardehrenfried approved these changes Jun 7, 2024

View reviewed changes

leonardehrenfried reviewed Jun 7, 2024

View reviewed changes

abyrd approved these changes Jun 11, 2024

View reviewed changes

optionsome merged commit 32bee57 into opentripplanner:dev-2.x Jun 11, 2024

optionsome deleted the fix-skipped-removal branch June 11, 2024 07:07

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

Add changelog entry for #5726 [ci skip]

51639fc

vpaturet mentioned this pull request Jul 1, 2024

Fix copy-on-write in TimetableSnapshot #5941

Merged

derhuerst mentioned this pull request Feb 20, 2025

strictly follow DIFFERENTIAL draft spec derhuerst/gtfs-rt-differential-to-full-dataset#1

Closed

Uh oh!

Conversation

optionsome commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Issue

Unit tests

Documentation

Changelog

Uh oh!

codecov Bot commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

t2gran commented Mar 8, 2024

Uh oh!

leonardehrenfried commented Mar 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

optionsome commented Mar 11, 2024

Uh oh!

optionsome commented Mar 14, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

optionsome Mar 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

optionsome commented Jun 3, 2024

Uh oh!

optionsome commented Jun 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

optionsome commented Jun 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

leonardehrenfried Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

optionsome commented Jun 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

optionsome commented Mar 4, 2024 •

edited

Loading

codecov Bot commented Mar 4, 2024 •

edited

Loading

optionsome Mar 15, 2024 •

edited

Loading

leonardehrenfried Jun 5, 2024 •

edited

Loading

leonardehrenfried Jun 5, 2024 •

edited

Loading