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.

Sign up for GitHub

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

Fix real-time added patterns persistence with DIFFERENTIAL updates #5726

Merged
merged 21 commits into from
Jun 11, 2024
Merged

Fix real-time added patterns persistence with DIFFERENTIAL updates #5726

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
e432a38
Fix issue with realtime added patterns persisting with DIFFERENTIAL u…
optionsome Mar 4, 2024
d3f467d
Only run removal of old leftovers if DIFFERENTIAL update
optionsome Mar 11, 2024
9630a4a
Set added trip as canceled again if canceled update
optionsome Mar 12, 2024
0177fb4
Add test for removing skipped update
optionsome Mar 12, 2024
f518363
Add tests for canceling/deleting added trips
optionsome Mar 14, 2024
dfe08af
Fix formatting
optionsome Mar 14, 2024
7cdbe3d
Use parametrized test
optionsome Mar 14, 2024
459348d
Combine multiple methods into one and clarify method naming/javadoc
optionsome Apr 29, 2024
b98c3c8
Rename parameter to better match its type
optionsome Apr 29, 2024
c653f56
Apply suggestions from code review
optionsome May 24, 2024
0aa8374
Fix formatting
optionsome May 24, 2024
4d73ce7
Merge remote-tracking branch 'upstream/dev-2.x' into fix-skipped-removal
optionsome May 24, 2024
e7ae470
More doc and method name changes based on review
optionsome May 24, 2024
fe8d52b
Merge remote-tracking branch 'upstream/dev-2.x' into fix-skipped-removal
optionsome Jun 4, 2024
b9526c3
Fix added trip cancellation/deletion logic
optionsome Jun 4, 2024
bdf7161
Remove some unnecessary param javadoc
optionsome Jun 4, 2024
1cb32d9
Clarify javadoc and only remove added removal when trip was added
optionsome Jun 4, 2024
51d7550
Merge remote-tracking branch 'upstream/dev-2.x' into fix-skipped-removal
optionsome Jun 4, 2024
441bd5c
Refactor new tests to be module tests
optionsome Jun 4, 2024
83da1d8
Clean up tests
optionsome Jun 7, 2024
bc06c14
Refactor and fix potential issue with added trip not being detected
optionsome Jun 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 1 addition & 22 deletions src/ext/java/org/opentripplanner/ext/siri/SiriTimetableSnapshotSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ private Result<TripUpdate, UpdateError> handleModifiedTrip(

// Also check whether trip id has been used for previously ADDED/MODIFIED trip message and
// remove the previously created trip
removePreviousRealtimeUpdate(trip, serviceDate);
this.buffer.removePreviousRealtimeUpdate(trip.getId(), serviceDate);

return updateResult;
}
Expand Down Expand Up @@ -410,27 +410,6 @@ private boolean markScheduledTripAsDeleted(Trip trip, final LocalDate serviceDat
return success;
}

/**
* Removes previous trip-update from buffer if there is an update with given trip on service date
*
* @param serviceDate service date
* @return true if a previously added trip was removed
*/
private boolean removePreviousRealtimeUpdate(final Trip trip, final LocalDate serviceDate) {
boolean success = false;

final TripPattern pattern = buffer.getRealtimeAddedTripPattern(trip.getId(), serviceDate);
if (pattern != null) {
// Remove the previous real-time-added TripPattern from buffer.
// Only one version of the real-time-update should exist
buffer.removeLastAddedTripPattern(trip.getId(), serviceDate);
buffer.removeRealtimeUpdatedTripTimes(pattern, trip.getId(), serviceDate);
success = true;
}

return success;
}

private boolean purgeExpiredData() {
final LocalDate today = LocalDate.now(transitModel.getTimeZone());
final LocalDate previously = today.minusDays(2); // Just to be safe...
Expand Down
94 changes: 57 additions & 37 deletions src/main/java/org/opentripplanner/model/TimetableSnapshot.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import org.opentripplanner.transit.model.network.TripPattern;
import org.opentripplanner.transit.model.site.StopLocation;
import org.opentripplanner.transit.model.timetable.TripIdAndServiceDate;
import org.opentripplanner.transit.model.timetable.TripOnServiceDate;
import org.opentripplanner.transit.model.timetable.TripTimes;
import org.opentripplanner.updater.spi.UpdateError;
import org.opentripplanner.updater.spi.UpdateSuccess;
Expand Down Expand Up @@ -111,38 +110,6 @@ public Timetable resolve(TripPattern pattern, LocalDate serviceDate) {
return pattern.getScheduledTimetable();
}

public void removeRealtimeUpdatedTripTimes(
optionsome marked this conversation as resolved.
Show resolved Hide resolved
TripPattern tripPattern,
FeedScopedId tripId,
LocalDate serviceDate
) {
SortedSet<Timetable> sortedTimetables = this.timetables.get(tripPattern);
if (sortedTimetables != null) {
TripTimes tripTimesToRemove = null;
for (Timetable timetable : sortedTimetables) {
if (timetable.isValidFor(serviceDate)) {
final TripTimes tripTimes = timetable.getTripTimes(tripId);
if (tripTimes == null) {
LOG.debug("No triptimes to remove for trip {}", tripId);
} else if (tripTimesToRemove != null) {
LOG.debug("Found two triptimes to remove for trip {}", tripId);
} else {
tripTimesToRemove = tripTimes;
}
}
}

if (tripTimesToRemove != null) {
for (Timetable sortedTimetable : sortedTimetables) {
boolean isDirty = sortedTimetable.getTripTimes().remove(tripTimesToRemove);
if (isDirty) {
dirtyTimetables.add(sortedTimetable);
}
}
}
}
}

/**
* Get the current trip pattern given a trip id and a service date, if it has been changed from
* the scheduled pattern with an update, for which the stopPattern is different.
Expand Down Expand Up @@ -295,11 +262,24 @@ public void clear(String feedId) {
}

/**
* Removes the latest added trip pattern from the cache. This should be done when removing the
* trip times from the timetable the trip has been added to.
* Removes previous trip-update from buffer if there is an update with given trip on service date
*
* @param serviceDate service date
Copy link
Member

Choose a reason for hiding this comment

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

It feels to me like this Javadoc and the name of this method might be misrepresenting its purpose. Please confirm if my understanding is correct. The method doesn't exactly remove a trip update, it removes some objects created only if a trip update with very particular characteristics has been applied. These are specifically a realtime-created TripPattern and the single trip that is expected to be present on that same TripPattern. And crucially, it only performs this removal if the supplied tripId has previously caused a new TripPattern to be created (conditional on the pattern != null block), which only happens when the stop sequence is changed by the update.

The Javadoc (and maybe the name) gives the impression that this method takes an action if any update has ever been applied to the given trip and service date combination, but the actual field of application is much narrower.

Aside: the @param serviceDate service date annotation seems trivial/redundant and should probably be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I agree with. I copied over the javadoc when I moved this method from the SiriTimetableSnapshotSource to here. I can update the javadoc to better match what the method does.

Copy link
Member

@t2gran t2gran Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Aside: the @param serviceDate service date annotation seems trivial/redundant and should probably be removed.

+1

* @return true if a previously added trip was removed
*/
public void removeLastAddedTripPattern(FeedScopedId feedScopedTripId, LocalDate serviceDate) {
realtimeAddedTripPattern.remove(new TripIdAndServiceDate(feedScopedTripId, serviceDate));
public boolean removePreviousRealtimeUpdate(FeedScopedId tripId, LocalDate serviceDate) {
t2gran marked this conversation as resolved.
Show resolved Hide resolved
boolean success = false;

final TripPattern pattern = getRealtimeAddedTripPattern(tripId, serviceDate);
if (pattern != null) {
// Remove the previous real-time-added TripPattern from buffer.
// Only one version of the real-time-update should exist
optionsome marked this conversation as resolved.
Show resolved Hide resolved
removeLastAddedTripPattern(tripId, serviceDate);
Copy link
Member

Choose a reason for hiding this comment

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

The private method removeLastAddedTripPattern is only one line long, is located in the same file, and is only called in this one place. Should it just be inlined here?

The private method removeRealtimeUpdatedTripTimes called on the next line is much longer, but similarly it's called only here in a method that's only a few lines long. Should it also be inlined here?

The tree of methods removePreviousRealtimeUpdate calling removeLastAddedTripPattern and removeRealtimeUpdatedTripTimes looks more complicated than it really is, and the whole tree is only called in 2 places. It's also a little confusing due to slight differences in the method names, raising doubts whether maybe "previous" means something different than "last added" and so on.

Copy link
Member Author

Choose a reason for hiding this comment

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

This sounds like a good idea. The mixture of different language around these "added trip patterns" is indeed slightly confusing.

Copy link
Member

Choose a reason for hiding this comment

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

+1 on better names, -1 on inlining removeRealtimeUpdatedTripTimes. One line methods have their place, even if only used once. If named properly they may explain/give an hint on the intention. A longer explination/description can go in the JavaDoc, which help keeping the caller method cleaner - shorter and at the same abstraction level.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can discuss this in the dev meeting.

removeRealtimeUpdatedTripTimes(pattern, tripId, serviceDate);
success = true;
}

return success;
}

/**
Expand Down Expand Up @@ -391,6 +371,46 @@ protected boolean clearRealtimeAddedTripPattern(String feedId) {
);
}

private void removeRealtimeUpdatedTripTimes(
TripPattern tripPattern,
FeedScopedId tripId,
LocalDate serviceDate
) {
SortedSet<Timetable> sortedTimetables = this.timetables.get(tripPattern);
if (sortedTimetables != null) {
TripTimes tripTimesToRemove = null;
for (Timetable timetable : sortedTimetables) {
if (timetable.isValidFor(serviceDate)) {
final TripTimes tripTimes = timetable.getTripTimes(tripId);
if (tripTimes == null) {
LOG.debug("No triptimes to remove for trip {}", tripId);
} else if (tripTimesToRemove != null) {
LOG.debug("Found two triptimes to remove for trip {}", tripId);
} else {
tripTimesToRemove = tripTimes;
}
}
}

if (tripTimesToRemove != null) {
for (Timetable sortedTimetable : sortedTimetables) {
boolean isDirty = sortedTimetable.getTripTimes().remove(tripTimesToRemove);
if (isDirty) {
dirtyTimetables.add(sortedTimetable);
}
}
}
}
}

/**
* Removes the latest added trip pattern from the cache. This should be done when removing the
Copy link
Member

Choose a reason for hiding this comment

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

Does this necessarily remove the trip pattern entirely from the cache? It seems to only remove the mapping from one particular TripIdAndServiceDate to the TripPattern, but the TripPattern could continue to be present for other keys.

The fact that this map is being referred to as a "cache" implies that the values are being reused, potentially for more than one tripId. I'm trying to establish whether there's an assumption throughout this part of the code that when a trip is changed such that it creates a new TripPattern, it's the only trip on that pattern.

Copy link
Member Author

@optionsome optionsome Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think you are correct. I can improve the javadoc.

Copy link
Member

@t2gran t2gran Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is one of the things I want to change with refactoring of the transit model. The goal should be:

  • One encapsulated call to add an entity (during graph build or RT). Then the one place will make sure the entity is inserted into the model and into any appropriate index. The model should be the master, and probably use events to update indexes. We can discuss design later if implementing it.

* trip times from the timetable the trip has been added to.
*/
private void removeLastAddedTripPattern(FeedScopedId feedScopedTripId, LocalDate serviceDate) {
realtimeAddedTripPattern.remove(new TripIdAndServiceDate(feedScopedTripId, serviceDate));
}

/**
* Add the patterns to the stop index, only if they come from a modified pattern
*/
Expand Down
54 changes: 30 additions & 24 deletions src/main/java/org/opentripplanner/updater/trip/TimetableSnapshotSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,17 @@ public UpdateResult applyTripUpdates(
serviceDate = localDateNow.get();
}

// Check whether trip id has been used for previously ADDED trip message and mark previously
// created trip as DELETED
var canceledPreviouslyAddedTrip = cancelPreviouslyAddedTrip(
tripId,
serviceDate,
CancelationType.DELETE
);
// Remove previous realtime updates for this trip. This is necessary to avoid previous
// stop pattern modifications from persisting
this.buffer.removePreviousRealtimeUpdate(tripId, serviceDate);

uIndex += 1;
LOG.debug("trip update #{} ({} updates) :", uIndex, tripUpdate.getStopTimeUpdateCount());
LOG.trace("{}", tripUpdate);
Expand All @@ -297,8 +308,18 @@ public UpdateResult applyTripUpdates(
tripId,
serviceDate
);
case CANCELED -> handleCanceledTrip(tripId, serviceDate, CancelationType.CANCEL);
case DELETED -> handleCanceledTrip(tripId, serviceDate, CancelationType.DELETE);
case CANCELED -> handleCanceledTrip(
tripId,
serviceDate,
CancelationType.CANCEL,
canceledPreviouslyAddedTrip
);
case DELETED -> handleCanceledTrip(
tripId,
serviceDate,
CancelationType.DELETE,
canceledPreviouslyAddedTrip
);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not too happy about passing down a boolean to method which leads to an immediate return and I would like to handle it earlier.

In an ideal world Java would have a syntax like this:

case CANCELED if canceledPreviouslyAddedTrip -> Result.success(UpdateSuccess.noWarnings());

but it doesn't.

How about catching that case before the switch? So something like

if (CANCELLED or DELETED) and canceledPreviouslyAddedTrip return Result.success(UpdateSuccess.noWarnings());

Copy link
Member Author

@optionsome optionsome Mar 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In general I agree with you. However, the handleCanceledTrip is where you expect the handling of any valid case of a CANCELED/DELETED scheduled relationship to be.

case REPLACEMENT -> validateAndHandleModifiedTrip(
tripUpdate,
tripDescriptor,
Expand Down Expand Up @@ -435,11 +456,6 @@ private Result<UpdateSuccess, UpdateError> handleScheduledTrip(
return UpdateError.result(tripId, NO_SERVICE_ON_DATE);
}

// If this trip_id has been used for previously ADDED/MODIFIED trip message (e.g. when the
// sequence of stops has changed, and is now changing back to the originally scheduled one),
// mark that previously created trip as DELETED.
cancelPreviouslyAddedTrip(tripId, serviceDate, CancelationType.DELETE);
Copy link
Member

Choose a reason for hiding this comment

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

Are you moving these call up because they happen for every scenario or is there another reason for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

There were two reasons, one of them was that I saw this was used in every scenario and the other was that I was unsure what would happen if I run the new code before this. However, I think moving this creates a minor regression as now the previously added trip is always deleted. However, I think previously if you sent a CANCELLED message on the trip, it was just cancelled, not removed. Do we want to keep that behavior?

Copy link
Member

Choose a reason for hiding this comment

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

So an ADDED trip was then CANCELLED again? If so, I'm quite relaxed about that but I would like to see tests documenting all these weird edge cases.

Copy link
Member

Choose a reason for hiding this comment

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

If it's just cancelling ADDDED trips, I think it doesn't matter.

Copy link
Member Author

Choose a reason for hiding this comment

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

@t2gran do you know how cancellation of an added trip works in SIRI? Do we cancel or remove it?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should cancel it. I have asked @lassetyr about this now, and in the future we want to be able to "cancel" any previously added update - DELETE in GTFS?

Summary of my thought about this - related to todays dev meeting discussion:

  • Trip updates is applied to the PLANED trips
    • not the previous updated trip(if it exist)
    • if the trip do not exist the update must create a new trip and set the appropriate flags. This mean in the future that we might need to support incomplete trips (trip update do not have a complete set of data).
  • To support scalability beyond the current demand, we might have to support incremental updates(incomplete trip data). If done, it needs to be combined with a "aggregator-service" witch keep track of the complete state and can serve the full-updates to OTP instance starting up, or resetting the RT state (periodically). For reference, look at the event-sourcing design pattern.


// Get new TripTimes based on scheduled timetable
var result = pattern
.getScheduledTimetable()
Expand Down Expand Up @@ -687,10 +703,6 @@ private Result<UpdateSuccess, UpdateError> handleAddedTrip(
"number of stop should match the number of stop time updates"
);

// Check whether trip id has been used for previously ADDED trip message and mark previously
// created trip as DELETED
cancelPreviouslyAddedTrip(tripId, serviceDate, CancelationType.DELETE);

Route route = getOrCreateRoute(tripDescriptor, tripId);

// Create new Trip
Expand Down Expand Up @@ -1105,10 +1117,6 @@ private Result<UpdateSuccess, UpdateError> handleModifiedTrip(
var tripId = trip.getId();
cancelScheduledTrip(tripId, serviceDate, CancelationType.DELETE);

// Check whether trip id has been used for previously ADDED/REPLACEMENT trip message and mark it
// as DELETED
cancelPreviouslyAddedTrip(tripId, serviceDate, CancelationType.DELETE);

// Add new trip
return addTripToGraphAndBuffer(
trip,
Expand All @@ -1123,19 +1131,17 @@ private Result<UpdateSuccess, UpdateError> handleModifiedTrip(
private Result<UpdateSuccess, UpdateError> handleCanceledTrip(
FeedScopedId tripId,
final LocalDate serviceDate,
CancelationType markAsDeleted
CancelationType markAsDeleted,
Copy link
Member

Choose a reason for hiding this comment

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

The variable name seems confusing - was this converted from a boolean at some point? Maybe it should just be called cancelationType.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can rename this.

boolean canceledPreviouslyAddedTrip
) {
// if previously a added trip was removed, there can't be a scheduled trip to remove
if (canceledPreviouslyAddedTrip) {
return Result.success(UpdateSuccess.noWarnings());
}
// Try to cancel scheduled trip
final boolean cancelScheduledSuccess = cancelScheduledTrip(tripId, serviceDate, markAsDeleted);

// Try to cancel previously added trip
final boolean cancelPreviouslyAddedSuccess = cancelPreviouslyAddedTrip(
tripId,
serviceDate,
markAsDeleted
);

if (!cancelScheduledSuccess && !cancelPreviouslyAddedSuccess) {
if (!cancelScheduledSuccess) {
debug(tripId, "No pattern found for tripId. Skipping cancellation.");
return UpdateError.result(tripId, NO_TRIP_FOR_CANCELLATION_FOUND);
}
Expand Down
Loading