Skip to content

Use SIRI-ET and GTFS-RT TripUpdates at the same time - #6363

Merged
leonardehrenfried merged 27 commits into
opentripplanner:dev-2.xfrom
leonardehrenfried:siri-gtfs
Feb 3, 2025
Merged

Use SIRI-ET and GTFS-RT TripUpdates at the same time#6363
leonardehrenfried merged 27 commits into
opentripplanner:dev-2.xfrom
leonardehrenfried:siri-gtfs

Conversation

@leonardehrenfried

@leonardehrenfried leonardehrenfried commented Jan 7, 2025

Copy link
Copy Markdown
Member

Summary

Previously it was not possible to run SIRI-ET and GTFS-RT TripUpdates in the same instance. This PR changes it by cleaning up the relationships of the various real-time components.

A class diagram to show the new relationships is available in the issue #6348.

Issue

Closes #6348
Ref #4002

Unit tests

Some unit tests updated.

Documentation

The new class diagram is available in this PR in doc/dev/diagrams/TimetableSnapshotManager.svg.

Bumping the serialization version id

While no actual entities have changed, but the TimetableRepository has which is serialized into the graph. Therefore we need to bump it.

cc @fahrplaner

@leonardehrenfried leonardehrenfried added !Technical Debt Improve code quality, no functional changes. +Real-Time The issue/PR is related to RealTime updates +Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR labels Jan 7, 2025
@leonardehrenfried
leonardehrenfried requested a review from a team as a code owner January 7, 2025 09:38
@codecov

codecov Bot commented Jan 7, 2025

Copy link
Copy Markdown

Codecov Report

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

Project coverage is 69.85%. Comparing base (12b5cb6) to head (b19e36e).
Report is 88 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...entripplanner/transit/configure/TransitModule.java 0.00% 5 Missing ⚠️
...ner/standalone/configure/ConstructApplication.java 0.00% 3 Missing ⚠️
...planner/updater/configure/UpdaterConfigurator.java 70.00% 3 Missing ⚠️
...oradapter/transit/mappers/TransitLayerUpdater.java 85.71% 0 Missing and 1 partial ⚠️
...planner/transit/service/DefaultTransitService.java 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6363      +/-   ##
=============================================
+ Coverage      69.77%   69.85%   +0.07%     
- Complexity     18053    18115      +62     
=============================================
  Files           2060     2069       +9     
  Lines          77122    77213      +91     
  Branches        7856     7842      -14     
=============================================
+ Hits           53814    53938     +124     
+ Misses         20556    20522      -34     
- Partials        2752     2753       +1     

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

@optionsome
optionsome requested a review from vpaturet January 9, 2025 14:42
@leonardehrenfried leonardehrenfried changed the title Use SIRI-ET and GTFS-RT TripUpates at the same time Use SIRI-ET and GTFS-RT TripUpdates at the same time Jan 25, 2025

@vpaturet vpaturet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks good. Just minor comments.

@vpaturet
vpaturet self-requested a review January 29, 2025 10:27
vpaturet
vpaturet previously approved these changes Jan 29, 2025

@vpaturet vpaturet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You probably don't need to bump the serialization id since you change only a transient field.

@leonardehrenfried

Copy link
Copy Markdown
Member Author

So removing fields doesn't count as breaking backwards-compatibility?

@t2gran t2gran added this to the 2.7 (next release) milestone Jan 29, 2025
@abyrd

abyrd commented Jan 31, 2025

Copy link
Copy Markdown
Member

So removing fields doesn't count as breaking backwards-compatibility?

I think the idea is that the transient field is not serialized, so the serialized graph is still compatible and will load without error.

@leonardehrenfried leonardehrenfried removed the +Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR label Jan 31, 2025
@leonardehrenfried

Copy link
Copy Markdown
Member Author

Alright then. The label is gone.

@abyrd abyrd 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.

Looking good so far. Just added a few questions that would reassure me I'm understanding what I'm seeing. In addition to those line comments, would it be correct to say that the refactoring described in the Javadoc on TimetableSnapshotManager has now been carried out within this PR? Should that Javadoc be updated?

@leonardehrenfried

Copy link
Copy Markdown
Member Author

Looking good so far. Just added a few questions that would reassure me I'm understanding what I'm seeing. In addition to those line comments, would it be correct to say that the refactoring described in the Javadoc on TimetableSnapshotManager has now been carried out within this PR? Should that Javadoc be updated?

These observations are absolutely correct and I have updated the Javadoc.

@abyrd abyrd 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.

Thanks for confirming those points. This looks like a step in the right direction to me, thanks for all your work on this. Understood about any renaming / documentation updates coming in a subsequent step, which I'd be happy to contribute to.

@leonardehrenfried
leonardehrenfried merged commit 1ebb824 into opentripplanner:dev-2.x Feb 3, 2025
t2gran pushed a commit that referenced this pull request Feb 3, 2025
@leonardehrenfried
leonardehrenfried deleted the siri-gtfs branch February 3, 2025 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

+Real-Time The issue/PR is related to RealTime updates !Technical Debt Improve code quality, no functional changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Using SIRI-ET and GTFS-RT TripUpdates in the same instance

4 participants