OTP-1807 Move Trivial MonitoredTrip PreChecks

src/main/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTrip.java

@@ -750,30 +750,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()) {
@@ -858,6 +834,50 @@ 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 on fields in a candidate monitored trip as obtained from the database, to determine
+     * whether to do all the other checking that instantiating this class has to offer.
+     * @param trip candidate monitored trip to quickly check some fields of
+     * @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;
+        }
+
+        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.
@@ -921,13 +941,6 @@ private static ZonedDateTime findNextMonitoredDay(MonitoredTrip trip, ZonedDateT
         return nextMonitoredDay;
     }
 
-    /**
-     * Is a one-off trip which has already happened.
-     */
-    private boolean isOneTimeTripInPast() {
-        return trip.isOneTime() && previousJourneyState.tripStatus == TripStatus.PAST_TRIP;
-    }
-
     /** Check if previous matching itinerary day is still valid */
     private boolean isPrevMatchingItineraryDayValid() {
         if (previousMatchingItinerary == null) return false;

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

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

src/test/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTripTest.java

@@ -1031,7 +1031,7 @@ void shouldReportRecurringTripInstanceInPastWithTrackingAsActive() throws Except
         String todayFormatted = trip.journeyState.targetDate;
 
         CheckMonitoredTrip check = new CheckMonitoredTrip(trip, this::mockOtpPlanResponse);
-        check.shouldSkipMonitoredTripCheck(false);
+        check.shouldSkipMonitoredTripCheck(false, true);
         check.checkOtpAndUpdateTripStatus();
         // Trip should remain active, and the target date should still be "today".
         assertEquals(TRIP_ACTIVE, trip.journeyState.tripStatus);
@@ -1215,13 +1215,15 @@ void canUpdateTripWithStaleState(
         // Mock the current time
         DateTimeUtils.useFixedClockAt(clockTime);
 
-        // After trip has completed, check that trip status has been updated.
-        CheckMonitoredTrip check = new CheckMonitoredTrip(monitoredTrip, this::mockOtpPlanResponse);
+        if (!CheckMonitoredTrip.shouldSkipMonitoredTripPreCheck(monitoredTrip)) {
+            // After trip has completed, check that trip status has been updated.
+            CheckMonitoredTrip check = new CheckMonitoredTrip(monitoredTrip, this::mockOtpPlanResponse);
 
-        check.run();
+            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<Arguments> createUpdateTripWithStaleStateCases() {
@@ -1266,17 +1268,19 @@ void shouldNotUpdateInactiveOrSnoozedTrip(
         // Mock the current time
         DateTimeUtils.useFixedClockAt(clockTime);
 
-        // After trip has completed, check that trip status has been updated.
-        CheckMonitoredTrip check = new CheckMonitoredTrip(monitoredTrip, this::mockOtpPlanResponse);
+        if (!CheckMonitoredTrip.shouldSkipMonitoredTripPreCheck(monitoredTrip)) {
+            // After trip has completed, check that trip status has been updated.
+            CheckMonitoredTrip check = new CheckMonitoredTrip(monitoredTrip, this::mockOtpPlanResponse);
 
-        check.run();
+            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<Arguments> shouldNotUpdateInactiveOrSnoozedTripCases() {