OTP-1807 Move Trivial MonitoredTrip PreChecks by JymDyerIBI · Pull Request #343 · ibi-group/otp-middleware

JymDyerIBI · 2025-08-28T21:28:08Z

Checklist

Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
The description lists all applicable issues this PR seeks to resolve
The description lists any configuration setting(s) that differ from the default settings (N/A)
All tests and CI builds passing

Description

CheckMonitoredTrip.shouldSkipMonitoredTripCheck() is a "big brain method" with many checks going on. There are simple checks that can be done as soon as a monitored trip is obtained, so move them into a new method.

In production, TripAnalyzer will obtain a monitored trip from MongoDB, then instantiate a CheckMonitoredTrip class and execute its run() method. We can call this new method before even instantiating CheckMonitoredTrip and avoid all that overhead.

…yzer can determine whether to even bother locking the monitored trip and all the setup for instantiating this class and running run(). - Convenience method for unit test to call static method.

…h preCheck = true for unit test.

…PreCheck() method to get the trivial checks out of the way before instantiating and running CheckMonitoredTrip instance.

…prechecks

…nitoredTripPreCheck().

…h a precheck.

br648

Checking if a snooze check can be added to the pre-check scope or not?

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

binh-dam-ibigroup · 2025-09-22T10:27:18Z

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

+     */
+    protected static boolean shouldSkipMonitoredTripPreCheck(MonitoredTrip trip) {
+        // before anything else, return true if the trip is inactive
+        if (!trip.isActive) {


Because no action is performed on trips that match at least the criteria in these methods, I think the criteria should be added to the Mongo filter in MonitorAllTripsJob.makeTripFilter (where some checks here are redundant with those filters). What do you think?

@JymDyerIBI (not blocking) Any thoughts on my comment above?

binh-dam-ibigroup · 2025-09-22T10:28:11Z

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

-        assertEquals(expectedStatus, modifiedTrip.journeyState.tripStatus, message);
+            MonitoredTrip modifiedTrip = Persistence.monitoredTrips.getById(monitoredTrip.id);
+            assertEquals(expectedStatus, modifiedTrip.journeyState.tripStatus, message);
+	}


Fix indent for if block.

Indent fixed.

Fix indent for the closing bracket.

binh-dam-ibigroup

Could we move stuff from shouldSkipMonitoredTripCheck(trip) to the Mongo filter in makeTripFilter?

…prechecks

binh-dam-ibigroup

Hi @JymDyerIBI, with the latest changes, it is no longer possible to snooze/unsnooze a trip or to modify a snoozed trip (getting request time outs in the UI).

binh-dam-ibigroup · 2025-10-09T18:32:01Z

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

Some code broke in CI with the error "CheckMonitoredTripTest.java:[1164,43] method shouldUnsnoozeTrip in class org.opentripplanner.middleware.tripmonitor.jobs.CheckMonitoredTrip cannot be applied to given types".

binh-dam-ibigroup · 2025-10-09T18:35:34Z

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

-        assertEquals(expectedStatus, modifiedTrip.journeyState.tripStatus, message);
+            MonitoredTrip modifiedTrip = Persistence.monitoredTrips.getById(monitoredTrip.id);
+            assertEquals(expectedStatus, modifiedTrip.journeyState.tripStatus, message);
+	}


Fix indent for the closing bracket.

binh-dam-ibigroup · 2025-10-09T19:02:58Z

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


-        // After trip has completed, check that trip status has been updated.
-        CheckMonitoredTrip check = new CheckMonitoredTrip(monitoredTrip, this::mockOtpPlanResponse);
+        if (!CheckMonitoredTrip.shouldSkipMonitoredTripPreCheck(monitoredTrip)) {


Would it make sense to replace this if block with an assertion on the expected value for shouldSkipMonitoredTripCheck?

binh-dam-ibigroup · 2025-10-09T19:05:36Z

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

+     */
+    protected static boolean shouldSkipMonitoredTripPreCheck(MonitoredTrip trip) {
+        // before anything else, return true if the trip is inactive
+        if (!trip.isActive) {


@JymDyerIBI (not blocking) Any thoughts on my comment above?

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

binh-dam-ibigroup · 2025-10-09T19:52:55Z

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

+                // 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;
+                }
+


Delete this block and move the call to shouldSkipMonitoredTripPreCheck, either back inside shouldSkipMonitoredTripCheck, or in doRun right before the call to shouldSkipMonitoredTripCheck.

This change as written causes an error when updating a trip from the UI, for instance to pause notifications or modify a paused trip. When a trip is updated (PUT request), new CheckMonitoredTrip(trip).run() gets run. Because the skipping logic is no longer run, it will try to compute the next active date of an inactive trip (and will never find one).

…CheckMonitoredTrip.java Wording. Co-authored-by: Binh Dam <56846598+binh-dam-ibigroup@users.noreply.github.com>

…prechecks

JymDyerIBI added 4 commits August 28, 2025 14:03

OTP-1807 - Put trivial "prechecks" into new static method so TripAnal…

1a08039

…yzer can determine whether to even bother locking the monitored trip and all the setup for instantiating this class and running run(). - Convenience method for unit test to call static method.

OTP-1807 - Call CheckMonitoredTrip.shouldSkipMonitoredTripCheck() wit…

40a74b7

…h preCheck = true for unit test.

OTP-1807 - Call CheckMonitoredTrip.shouldSkipMonitoredTripCheck() wit…

2dcd510

…h preCheck = true for unit test.

OTP-1807 - Call new static CheckMonitoredTrip.shouldSkipMonitoredTrip…

6618ec7

…PreCheck() method to get the trivial checks out of the way before instantiating and running CheckMonitoredTrip instance.

JymDyerIBI self-assigned this Aug 28, 2025

JymDyerIBI added 3 commits September 8, 2025 13:42

Merge remote-tracking branch 'origin/dev' into OTP-1807-move-trivial-…

ae692fc

…prechecks

OTP-1807 - Detect inactive trips with CheckMonitoredTrip.shouldSkipMo…

6dcbe8e

…nitoredTripPreCheck().

OTP-1807 - The NO_LONGER_POSSIBLE status is a "stale" case caught wit…

a2e1470

…h a precheck.

JymDyerIBI requested a review from binh-dam-ibigroup September 13, 2025 05:08

JymDyerIBI assigned binh-dam-ibigroup and unassigned JymDyerIBI Sep 13, 2025

JymDyerIBI requested a review from br648 September 15, 2025 15:46

br648 reviewed Sep 16, 2025

View reviewed changes

src/main/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTrip.java Outdated Show resolved Hide resolved

src/main/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTrip.java Show resolved Hide resolved

br648 assigned JymDyerIBI and unassigned binh-dam-ibigroup Sep 16, 2025

binh-dam-ibigroup reviewed Sep 22, 2025

View reviewed changes

binh-dam-ibigroup requested changes Sep 22, 2025

View reviewed changes

JymDyerIBI added 2 commits October 2, 2025 16:35

Merge remote-tracking branch 'origin/dev' into OTP-1807-move-trivial-…

d0339b8

…prechecks

OTP-1807 Snooze into "prechecks".

71f1547

binh-dam-ibigroup requested changes Oct 9, 2025

View reviewed changes

JymDyerIBI and others added 6 commits November 14, 2025 09:40

Update src/main/java/org/opentripplanner/middleware/tripmonitor/jobs/…

52c36d5

…CheckMonitoredTrip.java Wording. Co-authored-by: Binh Dam <56846598+binh-dam-ibigroup@users.noreply.github.com>

Update src/main/java/org/opentripplanner/middleware/tripmonitor/jobs/…

be84fd9

…CheckMonitoredTrip.java Wording. Co-authored-by: Binh Dam <56846598+binh-dam-ibigroup@users.noreply.github.com>

Merge branch 'dev' into OTP-1807-move-trivial-prechecks

34c33c6

Merge remote-tracking branch 'origin/dev' into OTP-1807-move-trivial-…

e914f22

…prechecks

OTP-1807 - Fix merge drift.

4d31203

Fix indent

41c71a8

Conversation

JymDyerIBI commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Description

Uh oh!

br648 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

Uh oh!

binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JymDyerIBI commented Aug 28, 2025 •

edited

Loading