Codecov Report

❌ Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.06%. Comparing base (bf2b9ac) to head (0f83c52).

@@            Coverage Diff             @@
##             dev-2.x    #6280   +/-   ##
==========================================
  Coverage      72.05%   72.06%           
- Complexity     20644    20649    +5     
==========================================
  Files           2231     2231           
  Lines          83543    83568   +25     
  Branches        8346     8348    +2     
==========================================
+ Hits           60198    60220   +22     
- Misses         20420    20422    +2     
- Partials        2925     2926    +1     

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

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

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.

What does this line actually mean:

My server log is flooded with this.

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);
}
}

2. 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
}

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);
}
}

2. 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.

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.

I have updated my approach as the original contained a mistake which broke the cancelled trip fetching.

I have now deferred the actual restoration of the timetable to the time of commit. When clear() is called, the pattern / service date pairs cleared are stored as a separate field, and during commit, the scheduled timetables of these pairs are added to the dirty set such that they are passed to the updater.

I would like to discuss this again given that #6909 didn't fix the bug.

@miklcct
It is clear that this is a real problem. We currently don't clear added trips for full dataset updates.

I think the correct thing would be to refactor the RaptorTransitData/RaptorTransitDataUpdater to natively support something like clear(String feedId), as @vpaturet was describing in a previous comment. But this would be a bigger refactor and would require redesigning the RaptorTransitData data structures.

The proposed fix looks like it will work and it is probably the best solution without refactoring the RaptorTransitData. But it adds complexity to the TimetableSnapshot where it doesn't really belong IMO, and the realtime code is already so full of technical debt. Therefore I would rather see that we did the more ambitious solution. We can talk about it on the next developer meeting you attend.

@miklcct I took a look at this last week and I agree with @habrahamsson-skanetrafiken that it's the cleanest we can currently do.

However, I think the tests could be a little cleaner, for that reason I opened #7129 which contains tests for the SIRI code and crucially a disabled test for this case. Once it is merged, I will review this one.

This PR also resolves two disabled test cases about new patterns being generated, which were recently added by @flaktack.