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 16 commits
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
24 changes: 1 addition & 23 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 @@ -268,7 +268,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.revertTripToScheduledTripPattern(trip.getId(), serviceDate);

return updateResult;
}
Expand All @@ -295,7 +295,6 @@ private Result<UpdateSuccess, UpdateError> addTripToGraphAndBuffer(TripUpdate tr
/**
* Mark the scheduled trip in the buffer as deleted, given trip on service date
*
* @param serviceDate service date
* @return true if scheduled trip was marked as deleted
*/
private boolean markScheduledTripAsDeleted(Trip trip, final LocalDate serviceDate) {
Expand All @@ -320,25 +319,4 @@ 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;
}
}
86 changes: 48 additions & 38 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 @@ -141,44 +141,10 @@ 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.
*
* @param tripId trip id
* @param serviceDate service date
* @return trip pattern created by the updater; null if trip is on the original trip pattern
*/
public TripPattern getRealtimeAddedTripPattern(FeedScopedId tripId, LocalDate serviceDate) {
Expand Down Expand Up @@ -325,11 +291,55 @@ 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.
* If a previous realtime update has changed which trip pattern is associated with the given trip
* on the given service date, this method will dissociate the trip from that pattern and remove
* the trip's timetables from that pattern on that particular service date.
*
* For this service date, the trip will revert to its original trip pattern from the scheduled
* data, remaining on that pattern unless it's changed again by a future realtime update.
*
* @return true if the trip was found to be shifted to a different trip pattern by a realtime
* message and an attempt was made to re-associate it with its originally scheduled trip pattern.
*/
public void removeLastAddedTripPattern(FeedScopedId feedScopedTripId, LocalDate serviceDate) {
realtimeAddedTripPattern.remove(new TripIdAndServiceDate(feedScopedTripId, serviceDate));
public boolean revertTripToScheduledTripPattern(FeedScopedId tripId, LocalDate serviceDate) {
boolean success = false;

final TripPattern pattern = getRealtimeAddedTripPattern(tripId, serviceDate);
if (pattern != null) {
// Dissociate the given trip from any realtime-added pattern.
// The trip will then fall back to its original scheduled pattern.
realtimeAddedTripPattern.remove(new TripIdAndServiceDate(tripId, serviceDate));
// Remove times for the trip from any timetables
// under that now-obsolete realtime-added pattern.
SortedSet<Timetable> sortedTimetables = this.timetables.get(pattern);
optionsome marked this conversation as resolved.
Show resolved Hide resolved
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);
}
}
}
}
success = true;
}

return success;
}

/**
Expand Down
86 changes: 51 additions & 35 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 @@ -190,15 +190,37 @@ public UpdateResult applyTripUpdates(
// starts for example at 40:00, yesterday would probably be a better guess.
serviceDate = localDateNow();
}

uIndex += 1;
LOG.debug("trip update #{} ({} updates) :", uIndex, tripUpdate.getStopTimeUpdateCount());
LOG.trace("{}", tripUpdate);

// Determine what kind of trip update this is
final TripDescriptor.ScheduleRelationship tripScheduleRelationship = determineTripScheduleRelationship(
tripDescriptor
);
var canceledPreviouslyAddedTrip = false;
if (!fullDataset) {
// Check whether trip id has been used for previously ADDED trip message and mark previously
// created trip as DELETED unless schedule relationship is CANCELED, then as CANCEL
var cancelationType = tripScheduleRelationship ==
TripDescriptor.ScheduleRelationship.CANCELED
? CancelationType.CANCEL
: CancelationType.DELETE;
canceledPreviouslyAddedTrip =
cancelPreviouslyAddedTrip(tripId, serviceDate, cancelationType);
// Remove previous realtime updates for this trip. This is necessary to avoid previous
// stop pattern modifications from persisting. If a trip was previously added with the ScheduleRelationship
// ADDED and is now cancelled or deleted, we still want to keep the realtime added trip pattern.
if (
!canceledPreviouslyAddedTrip ||
(
tripScheduleRelationship != TripDescriptor.ScheduleRelationship.CANCELED &&
tripScheduleRelationship != TripDescriptor.ScheduleRelationship.DELETED
)
) {
this.buffer.revertTripToScheduledTripPattern(tripId, serviceDate);
}
}
Copy link
Member

@leonardehrenfried leonardehrenfried Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can you move this into a method, please?

Copy link
Member

@leonardehrenfried leonardehrenfried Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, why don't you do all of this logic in handelCancelledTrip?

Copy link
Member

Choose a reason for hiding this comment

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

I just don't want to see half the logic on one level of abstraction and the other half in another one.


uIndex += 1;
LOG.debug("trip update #{} ({} updates) :", uIndex, tripUpdate.getStopTimeUpdateCount());
LOG.trace("{}", tripUpdate);

Result<UpdateSuccess, UpdateError> result;
try {
Expand All @@ -216,8 +238,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 @@ -327,11 +359,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 @@ -578,10 +605,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 @@ -827,8 +850,6 @@ private Result<UpdateSuccess, UpdateError> addTripToGraphAndBuffer(
/**
* Cancel scheduled trip in buffer given trip id on service date
*
* @param tripId trip id
* @param serviceDate service date
* @return true if scheduled trip was cancelled
*/
private boolean cancelScheduledTrip(
Expand Down Expand Up @@ -870,16 +891,14 @@ private boolean cancelScheduledTrip(
* exist, and will be reused if a similar added/modified trip message is received with the same
* route and stop sequence.
*
* @param tripId trip id without agency id
* @param serviceDate service date
* @return true if a previously added trip was cancelled
*/
private boolean cancelPreviouslyAddedTrip(
Copy link
Member Author

Choose a reason for hiding this comment

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

The naming of this method is slightly misleading.

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've limited what this method does so the naming is more correct now. I think this method was now otherwise doing duplicate stuff with the new clean-up method I've introduced to the GTFS RT logic.

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 now realize that 1cb32d9 commit title is unreadable but I don't think I'm going to fix it as we usually don't fix typos in commit titles 😅

final FeedScopedId tripId,
final LocalDate serviceDate,
CancelationType cancelationType
) {
boolean success = false;
boolean cancelledAddedTrip = false;

final TripPattern pattern = buffer.getRealtimeAddedTripPattern(tripId, serviceDate);
if (pattern != null) {
Expand All @@ -897,11 +916,10 @@ private boolean cancelPreviouslyAddedTrip(
case DELETE -> newTripTimes.deleteTrip();
}
buffer.update(pattern, newTripTimes, serviceDate);
success = true;
cancelledAddedTrip = pattern.getOriginalTripPattern() == null;
}
}

return success;
return cancelledAddedTrip;
}

/**
Expand Down Expand Up @@ -996,10 +1014,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 @@ -1014,19 +1028,21 @@ private Result<UpdateSuccess, UpdateError> handleModifiedTrip(
private Result<UpdateSuccess, UpdateError> handleCanceledTrip(
FeedScopedId tripId,
final LocalDate serviceDate,
CancelationType markAsDeleted
CancelationType cancelationType,
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(
final boolean cancelScheduledSuccess = cancelScheduledTrip(
tripId,
serviceDate,
markAsDeleted
cancelationType
);

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
Loading