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

Pass empty timetable to TransitLayerUpdater in order to clear added trip patterns #6280

Open
wants to merge 4 commits into
base: dev-2.x
Choose a base branch
from

Conversation

miklcct
Copy link
Contributor

@miklcct miklcct commented Nov 26, 2024

PR Instructions

Summary

Currently, after clearing TimetableSnapshot, if it is committed directly empty, nothing is passed to TransitLayerUpdater. As a result, previously added trip patterns remain in the TransitLayer for routing purpose, resulting in stale timetables returned in itinerary results which no longer exist in the actual, published timetable snapshot.

Issue

Fixes #6197

Unit tests

Added

Documentation

Bug fix only, not needed

Changelog

@miklcct miklcct force-pushed the fix-update-stale-service branch from 5a4a351 to d931077 Compare November 27, 2024 11:52
@miklcct miklcct marked this pull request as ready for review November 27, 2024 11:54
@miklcct miklcct requested a review from a team as a code owner November 27, 2024 11:54
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 84.09091% with 7 lines in your changes missing coverage. Please review.

Project coverage is 69.80%. Comparing base (684b841) to head (e09e5ba).
Report is 1 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...main/java/org/opentripplanner/model/Timetable.java 45.45% 2 Missing and 4 partials ⚠️
...a/org/opentripplanner/model/TimetableSnapshot.java 96.96% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             dev-2.x    #6280   +/-   ##
==========================================
  Coverage      69.79%   69.80%           
- Complexity     17786    17798   +12     
==========================================
  Files           2017     2017           
  Lines          76042    76081   +39     
  Branches        7781     7789    +8     
==========================================
+ Hits           53073    53106   +33     
  Misses         20264    20264           
- Partials        2705     2711    +6     

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

@leonardehrenfried leonardehrenfried changed the title Pass empty timetable to TransitLayerUpdater for cleared added trip patterns Pass empty timetable to TransitLayerUpdater in order to clear added trip patterns Nov 29, 2024
@t2gran t2gran requested a review from vpaturet November 29, 2024 10:36
@miklcct
Copy link
Contributor Author

miklcct commented Nov 30, 2024

Sorry it seems that the bug still appears on the live server, so I need to mark this as draft for now.

@miklcct miklcct marked this pull request as draft November 30, 2024 08:47
@miklcct
Copy link
Contributor Author

miklcct commented Nov 30, 2024

The solution will not work if a timetable is cleared multiple times between commits. Also on my live server log there are a lot of "WARN could not fetch timetable for pattern" which may be related to this problem as well. I'll need to further fix it.

@miklcct
Copy link
Contributor Author

miklcct commented Nov 30, 2024

What does this line actually mean:

My server log is flooded with this.

@miklcct miklcct marked this pull request as ready for review December 2, 2024 17:59
@vpaturet
Copy link
Contributor

vpaturet commented Dec 9, 2024

Would it work and would it be simpler if:

  1. We set a flag in the TransitLayerUpdater each time we call TimetableSnapshot.clear():
public clear(String feedId) {
...
if(transitLayerUpdater != null) {
transitLayerUpdater.setCleared(feedId);
}
}
  1. The TransitLayerUpdater checks the flag and clears all data belonging to this feed before applying the updates:

public void update(
    Collection<Timetable> updatedTimetables,
    Map<TripPattern, SortedSet<Timetable>> timetables
  ) {
if(isCleared(feedId)) {
realtimeTransitLayer.clear(feedId);
setCleared(feedId, false);
}
// apply updates
}

@miklcct
Copy link
Contributor Author

miklcct commented Dec 9, 2024

Would it work and would it be simpler if:

  1. We set a flag in the TransitLayerUpdater each time we call TimetableSnapshot.clear():
public clear(String feedId) {
...
if(transitLayerUpdater != null) {
transitLayerUpdater.setCleared(feedId);
}
}
  1. The TransitLayerUpdater checks the flag and clears all data belonging to this feed before applying the updates:

public void update(
    Collection<Timetable> updatedTimetables,
    Map<TripPattern, SortedSet<Timetable>> timetables
  ) {
if(isCleared(feedId)) {
realtimeTransitLayer.clear(feedId);
setCleared(feedId, false);
}
// apply updates
}

I think it will be better and I will work on this.

@miklcct
Copy link
Contributor Author

miklcct commented Dec 12, 2024

Unfortunately TransitLayerUpdater is not feed aware and it doesn't really have a state, so it is not possible for me to do that without adding complexity into the system. There is no concept of a "feed" and the "original" in the TransitLayer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stale services are used for journey planning
3 participants