Skip to content

Avoid cumulative real-time updates - #5705

Merged
t2gran merged 2 commits into
opentripplanner:dev-2.xfrom
entur:otp_fix_siri_update
Mar 4, 2024
Merged

Avoid cumulative real-time updates#5705
t2gran merged 2 commits into
opentripplanner:dev-2.xfrom
entur:otp_fix_siri_update

Conversation

@t2gran

@t2gran t2gran commented Feb 26, 2024

Copy link
Copy Markdown
Member

Summary

Realtime updates (at least Siri ET) should in general not be treated as cumulative updates. This make the logic complicated, you need to account for all kind of updates when applying a new on top of others. In the case of a malicious update, resending a ok one will not fix the problem as well.

This PR changes the stop-pattern logic so that a update is applied on top of the planned stop-pattern and not the last version from the previous update. There are a few places where this happens, so I included a few commits witch revert the changes if needed. The plan is either to drop those commits or to squash them. (I have removed these commits now, since no one requested them).

I would like to see som tests of this with various updators:

  • Siri (Entur)
  • Siri (Skaanetrafiken)
  • GTFS (HSL)
  • StopConsolidation - IBI (Sandbox)

Issue

At Entur we applied the following update several time and OTP toggled between the expected state (Stop 1,,2,3 CANCELED) and not (Stop 1,2,3 SCHEDULED).

Netex data set - One line
Siri update

To reproduce the problem, you can start OTP with the given data set and use the following build-config.json

"updaters": [
 {
      "type": "siri-et-updater",
      "url": "file://[PATH-TO-LOCAL-FOLDER-WITH-SIRI-XML-FILE]",
      "frequency": "3s",
      "previewInterval": "3h",
      "feedId": "RB",
      "blockReadinessUntilInitialized": true,
      "fuzzyTripMatching": true
     }
  ]

Possible challanges

Existing systems may relay on cumulative updates.

Unit tests

✅ One unit tests is added, and a few updated.

Documentation

Only JavaDoc

Changelog

Bumping the serialization version id

🟥 Not needed

@t2gran t2gran added !Bug Apply to issues describing a bug and PRs witch fixes it. Entur Test This is currently being tested at Entur labels Feb 26, 2024
@t2gran t2gran added this to the 2.5 (next release) milestone Feb 26, 2024
@t2gran
t2gran requested a review from a team as a code owner February 26, 2024 16:15
@t2gran t2gran changed the title Otp fix siri update Avoid cumulative real-time updates Feb 26, 2024
@codecov

codecov Bot commented Feb 26, 2024

Copy link
Copy Markdown

Codecov Report

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

Project coverage is 67.71%. Comparing base (e35ac67) to head (ed3d05a).
Report is 56 commits behind head on dev-2.x.

Files Patch % Lines
...tripplanner/transit/model/network/StopPattern.java 66.66% 6 Missing and 1 partial ⚠️
...tripplanner/transit/model/network/TripPattern.java 33.33% 2 Missing and 2 partials ⚠️
.../opentripplanner/ext/siri/ModifiedTripBuilder.java 84.61% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5705      +/-   ##
=============================================
+ Coverage      67.69%   67.71%   +0.01%     
- Complexity     16369    16410      +41     
=============================================
  Files           1890     1896       +6     
  Lines          71812    71964     +152     
  Branches        7407     7419      +12     
=============================================
+ Hits           48611    48728     +117     
- Misses         20692    20726      +34     
- Partials        2509     2510       +1     

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

@optionsome optionsome added the Digitransit Test Feature is under testing in Digitransit environment(s) label Feb 26, 2024
@leonardehrenfried

Copy link
Copy Markdown
Member

I'm frankly suprised that the updates are cumulative and would have expected (at least in the GTFS-RT case) it not to be the case, so I think this is the correct behaviour.

@leonardehrenfried leonardehrenfried left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this with several IBI data sets and all worked fine, including the stop consolidation.

However, I don't think anybody was ever relying on updates to stop patterns being cumulative.

Comment thread src/main/java/org/opentripplanner/framework/lang/MemEfficientArrayBuilder.java Outdated
Comment thread src/main/java/org/opentripplanner/framework/lang/MemEfficientArrayBuilder.java Outdated
Comment thread src/main/java/org/opentripplanner/framework/lang/MemEfficientArrayBuilder.java Outdated
Comment thread src/main/java/org/opentripplanner/transit/model/network/TripPattern.java Outdated
@jtorin

jtorin commented Feb 27, 2024

Copy link
Copy Markdown
Contributor

Found some minor spelling while reading through the PR.

@t2gran t2gran removed the Entur Test This is currently being tested at Entur label Feb 29, 2024
Co-authored-by: Johan Torin <jtorin@users.noreply.github.com>
@optionsome optionsome added the +Real-Time The issue/PR is related to RealTime updates label Mar 1, 2024
@t2gran
t2gran merged commit 41ffc95 into opentripplanner:dev-2.x Mar 4, 2024
@t2gran
t2gran deleted the otp_fix_siri_update branch March 4, 2024 14:06
t2gran pushed a commit that referenced this pull request Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Bug Apply to issues describing a bug and PRs witch fixes it. Digitransit Test Feature is under testing in Digitransit environment(s) +Real-Time The issue/PR is related to RealTime updates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants