Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify timetable snapshot handling for GTFS-RT and Siri #5853

Merged

Conversation

leonardehrenfried
Copy link
Member

@leonardehrenfried leonardehrenfried commented May 17, 2024

Summary

We discussed in one of the realtime meetings that the GTFS-RT and Siri code needs to be unified. This PR makes one step into that direction by introducing an abstract base class for handling the timetable snaphots. This is then used by both the Siri and GTFS-RT updaters.

Previously their handling of the snapshot was very similar, but not identical: Siri would always initialise a snapshot in the constructor but GTFS-RT would not. This meant that I had to modify the tests ever so slightly to make them pass, reflecting the fact that GTFS-RT now also initialises a snapshot during instantiation.

Further work

While using a base class has improved encapsulation and the separation of input mapping code from "framework code" (many fields are now private to the base class), the buffer is still very much exposed. In further work I would like to improve this further by totally encapsulating the buffer.

In the end I see design like the following:

  • split Siri/GTFS-RT updating code from the management of the snapshots
  • instead of base class, there is a timetable snapshot manager taking care of buffering and snapshoting
  • there is only one snapshot manager per transit model
  • the Siri/GTFS-RT updaters have a reference to this snapshot manager
  • Siri/GTFS-RT updaters can then update the snapshots sequentially

Unit tests

Unit tests were updated. Slight modifications were necessary.

Other work

#5852 and #5726 also touch on very similar code and I will wait until they are merged before requesting a code review.

@leonardehrenfried leonardehrenfried requested a review from a team as a code owner May 17, 2024 09:00
@leonardehrenfried leonardehrenfried added the Real-Time Update The issue/PR is related to RealTime updates label May 17, 2024
@leonardehrenfried leonardehrenfried changed the title Unify timetable snapshot handling in GTFS-RT and Siri Unify timetable snapshot handling for GTFS-RT and Siri May 17, 2024
Copy link

codecov bot commented May 17, 2024

Codecov Report

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

Project coverage is 68.49%. Comparing base (8f632f1) to head (2d86fc6).
Report is 1 commits behind head on dev-2.x.

Files Patch % Lines
.../updater/trip/AbstractTimetableSnapshotSource.java 95.34% 1 Missing and 1 partial ⚠️
...pplanner/ext/siri/SiriTimetableSnapshotSource.java 85.71% 1 Missing ⚠️
...a/org/opentripplanner/model/TimetableSnapshot.java 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5853      +/-   ##
=============================================
+ Coverage      68.47%   68.49%   +0.01%     
- Complexity     16698    16703       +5     
=============================================
  Files           1914     1915       +1     
  Lines          72673    72652      -21     
  Branches        7454     7447       -7     
=============================================
- Hits           49763    49760       -3     
+ Misses         20340    20326      -14     
+ Partials        2570     2566       -4     

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

@leonardehrenfried leonardehrenfried force-pushed the unify-timetable-snapshots branch from 497a830 to d9a68d5 Compare May 22, 2024 07:29
@leonardehrenfried
Copy link
Member Author

leonardehrenfried commented May 22, 2024

I've now rebased onto dev-2.x which contains @habrahamsson-skanetrafiken's Siri tests and they all still pass.

@leonardehrenfried leonardehrenfried force-pushed the unify-timetable-snapshots branch 3 times, most recently from 0a2d47c to e888149 Compare May 23, 2024 19:20
@leonardehrenfried leonardehrenfried force-pushed the unify-timetable-snapshots branch from f7a9ccd to 3bc32bb Compare May 24, 2024 20:35
@leonardehrenfried leonardehrenfried force-pushed the unify-timetable-snapshots branch from 3bc32bb to c1ea7a2 Compare May 27, 2024 11:43
@leonardehrenfried leonardehrenfried force-pushed the unify-timetable-snapshots branch from 26ed663 to 3c58564 Compare May 27, 2024 12:03
@vpaturet vpaturet self-requested a review May 28, 2024 09:13
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for making this as easy to review as possible! It looks like a good first step in unifying the snapshot stuff.

@leonardehrenfried leonardehrenfried force-pushed the unify-timetable-snapshots branch from 7ec3ad2 to 619e7cf Compare May 28, 2024 11:54
Copy link
Contributor

@vpaturet vpaturet left a comment

Choose a reason for hiding this comment

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

This looks good. Just a few typos.

@leonardehrenfried leonardehrenfried merged commit 17717d4 into opentripplanner:dev-2.x May 29, 2024
5 checks passed
@leonardehrenfried leonardehrenfried deleted the unify-timetable-snapshots branch May 29, 2024 10:39
@t2gran t2gran added this to the 2.6 (next release) milestone Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Real-Time Update The issue/PR is related to RealTime updates Skip Changelog Technical Debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants