diff --git a/src/main/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTrip.java b/src/main/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTrip.java index 097e09660..9eacdcacb 100644 --- a/src/main/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTrip.java +++ b/src/main/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTrip.java @@ -761,47 +761,6 @@ public boolean shouldSkipMonitoredTripCheck() throws Exception { } public boolean shouldSkipMonitoredTripCheck(boolean persist) throws Exception { - // before anything else, return true if the trip is inactive - if (!trip.isActive) { - LOG.info("Skipping: Trip is inactive."); - return true; - } - - // If trip is no longer possible, no further checking is needed. The itinerary existence data should not be - // checked here to avoid incorrectly skipping trips that are monitored on a single day of the week, but which - // may have not had a matching itinerary on that day for one week (even though the trip could be possible the - // next week). - if (previousJourneyState.tripStatus == TripStatus.NO_LONGER_POSSIBLE) { - LOG.info("Skipping: Trip is no longer possible."); - return true; - } - - // If trip is one-time and has ended, no further checking is needed. The itinerary existence data should not be - // checked here to avoid incorrectly skipping trips that are monitored on a single day of the week, but which - // may have not had a matching itinerary on that day for one week (even though the trip could be possible the - // next week). - if (isOneTimeTripInPast()) { - LOG.info("Skipping: One-time trip is in the past."); - return true; - } - - // For trips that are snoozed, see if they should be unsnoozed first. - if (trip.snoozed) { - if (shouldUnsnoozeTrip()) { - // Reset previous matching itineraries and start afresh. - previousMatchingItinerary = matchingItinerary = trip.itinerary.clone(); - - computeTargetZonedDateTime(); - matchingItinerary.offsetTimes(targetZonedDateTime); - - // Unsnooze trip now, for cases where the next itinerary isn't calculated. - trip.snoozed = false; - } else { - LOG.info("Skipping: Trip is snoozed."); - return true; - } - } - // initialize the trip's journey state and matching itinerary to the latest journeyState's matching // itinerary, or use the itinerary that the trip was saved with if (previousMatchingItinerary == null) { @@ -873,6 +832,67 @@ public boolean shouldSkipMonitoredTripCheck(boolean persist) throws Exception { return true; } + /** + * Convenience method for unit tests to do the precheck before the full check + */ + protected boolean shouldSkipMonitoredTripCheck(boolean persist, boolean preCheck) throws Exception { + if (preCheck && CheckMonitoredTrip.shouldSkipMonitoredTripPreCheck(trip)) { + return true; + } + return shouldSkipMonitoredTripCheck(persist); + } + + /** + * A series of trivial checks a {@pre MonitoredTrip} object, just using fields obtained from the database, to + * determine whether to instantiate a {@pre CheckMonitoredTrip} object to do further checking. + * @param trip candidate monitored trip + * @return whether to skip the trip, boolean value + */ + protected static boolean shouldSkipMonitoredTripPreCheck(MonitoredTrip trip) { + // before anything else, return true if the trip is inactive + if (!trip.isActive) { + LOG.info("Skipping: Trip {} is inactive.", trip.id); + return true; + } + + // If trip is no longer possible, no further checking is needed. The itinerary existence data should not be + // checked here to avoid incorrectly skipping trips that are monitored on a single day of the week, but which + // may have not had a matching itinerary on that day for one week (even though the trip could be possible the + // next week). + if (trip.journeyState.tripStatus == TripStatus.NO_LONGER_POSSIBLE) { + LOG.info("Skipping: Trip {} is no longer possible.", trip.id); + return true; + } + + // If trip is a one-off trip which has already happened, no further checking is needed. The itinerary existence + // data should not be checked here to avoid incorrectly skipping trips that are monitored on a single day of + // the week, but which may have not had a matching itinerary on that day for one week (even though the trip + // could be possible the next week). + if (trip.isOneTime() && trip.journeyState.tripStatus == TripStatus.PAST_TRIP) { + LOG.info("Skipping: One-time trip {} is in the past.", trip.id); + return true; + } + + // For trips that are snoozed, see if they should be unsnoozed first. + if (trip.snoozed) { + if (shouldUnsnoozeTrip(trip.journeyState.lastCheckedEpochMillis)) { + // Clear previous matching itinerary as we want to start afresh. + trip.journeyState.matchingItinerary = null; + // unsnooze trip now, for cases where the next itinerary isn't calculated + trip.snoozed = false; + } else { + LOG.info("Skipping: Trip {} is snoozed.", trip.id); + return true; + } + } + + return false; + } + + /** + * Calculate target time for the next trip plan request. Find the next possible day the trip is active by + * initializing the appropriate target time. + */ private void computeTargetZonedDateTime() { targetZonedDateTime = trip.computeTargetZonedDateTime(matchingItinerary); } @@ -881,6 +901,42 @@ private boolean isPreviousTripOngoingAtLastCheck() { return isPrevMatchingItineraryNotConcludedAtLastCheck() && isPrevMatchingItineraryDayValid(); } + /** + * Find, starting from the given date, the earliest target date for a monitored trip, + * using the trip start time and the monitored days. + * (Itinerary existence is not being checked, assuming that clients prevent monitoring days when a trip doesn't exist.) + */ + public static ZonedDateTime findEarliestTargetDate(MonitoredTrip trip, ZonedDateTime fromDateTime) { + ZonedDateTime itineraryEndTimeToday = makeOtpZonedDateTime( + fromDateTime, + trip.itinerary.endTime.toInstant() + ); + + int daysToAdd = fromDateTime.toInstant().isAfter(itineraryEndTimeToday.toInstant()) ? 1 : 0; + + ZonedDateTime nextStartDay = makeOtpZonedDateTime( + fromDateTime.plusDays(daysToAdd), + trip.itinerary.startTime.toInstant() + ); + + return findNextMonitoredDay(trip, nextStartDay); + } + + /** + * Advance the target date/time until a day is found when the trip is active. + */ + private static ZonedDateTime findNextMonitoredDay(MonitoredTrip trip, ZonedDateTime startingDay) { + ZonedDateTime nextMonitoredDay = startingDay; + if (!trip.isOneTime()) { + while (!trip.isActiveOnDate(nextMonitoredDay)) { + nextMonitoredDay = nextMonitoredDay.plusDays(1); + } + } + + return nextMonitoredDay; + } + + /** * Is a one-off trip which has already happened. */ @@ -994,14 +1050,15 @@ private Locale getOtpUserLocale() { /** * Whether a trip should be unsnoozed and monitoring should resume. + * @param lastCheckedEpochMillis to acquire the trip's start day * @return true if the current time is after the calendar day (on or after midnight) * after the matching trip start day, false otherwise. */ - public boolean shouldUnsnoozeTrip() { + public static boolean shouldUnsnoozeTrip(long lastCheckedEpochMillis) { ZoneId otpZoneId = DateTimeUtils.getOtpZoneId(); var midnightAfterLastChecked = ZonedDateTime .ofInstant( - Instant.ofEpochMilli(previousJourneyState.lastCheckedEpochMillis).plus(1, ChronoUnit.DAYS), + Instant.ofEpochMilli(lastCheckedEpochMillis).plus(1, ChronoUnit.DAYS), otpZoneId ) .withHour(0) diff --git a/src/main/java/org/opentripplanner/middleware/tripmonitor/jobs/TripAnalyzer.java b/src/main/java/org/opentripplanner/middleware/tripmonitor/jobs/TripAnalyzer.java index b25bc1f7f..50ee3d1f2 100644 --- a/src/main/java/org/opentripplanner/middleware/tripmonitor/jobs/TripAnalyzer.java +++ b/src/main/java/org/opentripplanner/middleware/tripmonitor/jobs/TripAnalyzer.java @@ -72,6 +72,13 @@ public void run() { continue; } + // Do a few trivial prechecks to see whether to even bother locking the trip + // and instantiating a CheckMonitoredTrip run. + if (CheckMonitoredTrip.shouldSkipMonitoredTripPreCheck(trip)) { + analyzerIsIdle.set(true); + continue; + } + LOG.info("Analyzing trip {}", tripId); // place lock on trip diff --git a/src/test/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTripBasicTest.java b/src/test/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTripBasicTest.java index 3d45a43fb..189e41759 100644 --- a/src/test/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTripBasicTest.java +++ b/src/test/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTripBasicTest.java @@ -43,7 +43,7 @@ void testSkipMonitoredTripCheck(SkipMonitoringTestArgs args) throws Exception { } assertEquals( args.expectedResult, - new CheckMonitoredTrip(trip).shouldSkipMonitoredTripCheck(false), + new CheckMonitoredTrip(trip).shouldSkipMonitoredTripCheck(false, true), args.message ); } diff --git a/src/test/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTripTest.java b/src/test/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTripTest.java index c020b3ff3..26b3c41ee 100644 --- a/src/test/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTripTest.java +++ b/src/test/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTripTest.java @@ -1356,7 +1356,7 @@ void canUnsnoozeTrip(ZonedDateTime lastCheckedTime, ZonedDateTime clockTime, boo check.previousJourneyState = journeyState; check.previousMatchingItinerary = monitoredTrip.itinerary; - assertEquals(shouldUnsnooze, check.shouldUnsnoozeTrip()); + assertEquals(shouldUnsnooze, check.shouldUnsnoozeTrip(journeyState.lastCheckedEpochMillis)); check.shouldSkipMonitoredTripCheck(); MonitoredTrip modifiedTrip = Persistence.monitoredTrips.getById(monitoredTrip.id); @@ -1418,12 +1418,14 @@ void canUpdateTripWithStaleState( // Mock the current time DateTimeUtils.useFixedClockAt(clockTime); - // After trip has completed, check that trip status has been updated. - CheckMonitoredTrip check = tripChecker(monitoredTrip, firstItinerary(mockOtpPlanResponse()), legDay.toLocalDate()); - check.run(); + if (!CheckMonitoredTrip.shouldSkipMonitoredTripPreCheck(monitoredTrip)) { + // After trip has completed, check that trip status has been updated. + CheckMonitoredTrip check = tripChecker(monitoredTrip, firstItinerary(mockOtpPlanResponse()), legDay.toLocalDate()); + check.run(); - MonitoredTrip modifiedTrip = Persistence.monitoredTrips.getById(monitoredTrip.id); - assertEquals(expectedStatus, modifiedTrip.journeyState.tripStatus, message); + MonitoredTrip modifiedTrip = Persistence.monitoredTrips.getById(monitoredTrip.id); + assertEquals(expectedStatus, modifiedTrip.journeyState.tripStatus, message); + } } private static Stream createUpdateTripWithStaleStateCases() { @@ -1464,16 +1466,18 @@ void shouldNotUpdateInactiveOrSnoozedTrip( // Mock the current time DateTimeUtils.useFixedClockAt(clockTime); - // After trip has completed, check that trip status has been updated. - CheckMonitoredTrip check = tripChecker(monitoredTrip, firstItinerary(mockOtpPlanResponse())); - check.run(); + if (!CheckMonitoredTrip.shouldSkipMonitoredTripPreCheck(monitoredTrip)) { + // After trip has completed, check that trip status has been updated. + CheckMonitoredTrip check = tripChecker(monitoredTrip, firstItinerary(mockOtpPlanResponse())); + check.run(); - MonitoredTrip modifiedTrip = Persistence.monitoredTrips.getById(monitoredTrip.id); - assertEquals( - monitoredTrip.journeyState.lastCheckedEpochMillis, - modifiedTrip.journeyState.lastCheckedEpochMillis, - "Should not check trip if " + (isActive ? "" : "in") + "active and " + (isSnoozed ? "" : "not ") + "snoozed." - ); + MonitoredTrip modifiedTrip = Persistence.monitoredTrips.getById(monitoredTrip.id); + assertEquals( + monitoredTrip.journeyState.lastCheckedEpochMillis, + modifiedTrip.journeyState.lastCheckedEpochMillis, + "Should not check trip if " + (isActive ? "" : "in") + "active and " + (isSnoozed ? "" : "not ") + "snoozed." + ); + } } private static Stream shouldNotUpdateInactiveOrSnoozedTripCases() {