Fall back to tripStatus deviation for arrival prediction - #1460
Ahmedhossamdev wants to merge 1 commit into
Conversation
|
Warning Review limit reachedNext included review available in 52 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe arrival endpoints now use predicted trip-status schedule deviation when direct stop-time predictions are unavailable. The change updates singular and plural endpoint tests for vehicle-position and downstream-only TripUpdate scenarios. ChangesArrival prediction fallback
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant ArrivalEndpoint
participant PredictionLookup
participant TripStatus
Client->>ArrivalEndpoint: request arrival data
ArrivalEndpoint->>PredictionLookup: retrieve direct predicted times
PredictionLookup-->>ArrivalEndpoint: no per-stop prediction
ArrivalEndpoint->>TripStatus: read predicted status and schedule deviation
TripStatus-->>ArrivalEndpoint: return schedule deviation
ArrivalEndpoint-->>Client: return predicted arrival and departure times
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The arrival-level `predicted` flag was derived only from getPredictedTimes, which requires a TripUpdate with a per-stop entry matching the queried stop, an earlier stop, or a trip-level Delay. BuildTripStatus already treats a trip as predicted whenever any real-time signal exists (fresh vehicle or any real-time source in the block). When the two paths disagreed, the same response would report `tripStatus.predicted=true` and `arrival.predicted= false` with `predictedArrivalTime=0`, contradicting Java. Add a fallback that runs after getPredictedTimes misses: if tripStatus.Predicted is set, mark the arrival predicted and use scheduledTime + tripStatus.ScheduleDeviation as the predicted time. This mirrors Java's setPredictedTimesFromScheduleDeviation, which runs when setPredictedTimesFromTimepointPredictionRecords fails. Flip two tests that codified the old strict behavior and add coverage for a TripUpdate whose only StopTimeUpdate targets a downstream stop.
6c54353 to
d736bb1
Compare
|
Performance Smoke Test ResultsStatus: PASSED
Smoke test config: 5 VUs x 30s. Thresholds: p(95) < 300ms, error rate < 1%. Full results uploaded as workflow artifact: k6-smoke-summary. |
1 participant
Fixes: #1456
Bug
The same response could disagree with itself.
tripStatus.predictedsaidtrueandtripStatus.scheduleDeviationwas populated, but the arrival's ownpredictedfield saidfalsewithpredictedArrivalTime=0. Java doesn't do this: it uses one flag for both.Concrete case from the issue, stop
1_1013, trip1_2008464:tripStatus.predictedtripStatus.scheduleDeviationpredicted(top-level)predictedArrivalTimeA client filtering on
arrival.predictedtreated a real-time-tracked bus as schedule-only. A client relying onpredictedArrivalTime > 0fell back to the scheduled time even though the deviation was right there on the same object.Why
Two independent code paths were computing "is this predicted":
getPredictedTimes(per-stop): true only if the TripUpdate has an STU for this stop, an earlier stop, or a trip-levelDelay.BuildTripStatus(whole-trip): true whenever any real-time signal exists (fresh vehicle OR any StopTimeUpdate anywhere in the block).For HART trips whose TripUpdates only carry per-stop entries for stops past the queried one,
BuildTripStatusstill resolved aScheduleDeviationvia its closest-in-time STU fallback, butgetPredictedTimesreturned false. The response then contradicted itself.Java uses one signal for both:
blockLocation.isPredicted()drives the arrival'spredictedflag, andsetPredictedTimesFromScheduleDeviationfills the predicted times asscheduled + deviationwhen the per-stop path (setPredictedTimesFromTimepointPredictionRecords) fails. SeeArrivalsAndDeparturesBeanServiceImpl.applyBlockLocationToBeanandArrivalAndDepartureServiceImpl.setPredictedTimesFromScheduleDeviation.Fix
Add a fallback in both handlers: when
getPredictedTimesreturnsfalsebuttripStatus.Predictedistrue, mark the arrival predicted and set predicted times toscheduled + tripStatus.ScheduleDeviation.predictedTimesFromTripStatusintrips_helper.go.buildArrivalinarrivals_core.go(plural handler) and the singular handler.TestPluralArrivals_NoMatchingOrPriorStop,TestPluralArrivals_VehiclePositionAloneDoesNotPredict); flipped both to assert the new behavior._DownstreamOnlySTUPredictsFromDeviationtests (one per handler) that reproduce the exact issue scenario.Verification
Live diff against Tampa hosted Java (same HART feed) for stop
1_1013:predicted=false, predictedArrivalTime=0on the three tracked trips at that stop.predicted=true, predictedArrivalTime=scheduled+deviation, matching Java.Summary by CodeRabbit