refactor: remove routesMap and migrate route lookups to DB#815
refactor: remove routesMap and migrate route lookups to DB#815Ahmedhossamdev wants to merge 2 commits intomainfrom
Conversation
…e queries - Removed routesMap from Manager struct and related methods. - Updated FindRoute to fetch routes from the database. - Refactored filtering functions to use a routeAgencyMap for agency resolution. - Enhanced updateFeedRealtime to build routeAgencyMap from the database. - Simplified tests to align with the new agency filtering logic.
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. |
fletcherw
left a comment
fletcherw
left a comment
There was a problem hiding this comment.
I think this is on the right path, it can be simplified by inverting the filtering query (from finding agencies for routes to finding routes for agencies).
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. |
fletcherw
left a comment
fletcherw
left a comment
There was a problem hiding this comment.
looks good, one small nit and a rebase is all that's needed.
| var trips []gtfs.Trip | ||
| var vehicles []gtfs.Vehicle | ||
| var alerts []gtfs.Alert | ||
| if tripData != nil && tripErr == nil { |
There was a problem hiding this comment.
nit: these three ifs can be merged with their corresponding if blocks on line 323, for example:
if len(agencyFilter) > 0 {
var queries *gtfsdb.Queries
if manager.GtfsDB != nil {
queries = manager.GtfsDB.Queries
}
allowedRouteIDs := buildAllowedRouteIDs(ctx, queries, agencyFilter)
if tripData != nil && tripErr == nil {
tripData.Trips = filterTripsByAgency(tripData.Trips, allowedRouteIDs)
}
[...]
| } | ||
| routeIDs := make(map[string]bool) | ||
| for agencyID := range allowed { | ||
| ids, err := queries.GetRouteIDsForAgency(ctx, agencyID) |
There was a problem hiding this comment.
not for this PR, but we should consider a bulk query for this.
| manager.gtfsData = newStaticData | ||
| manager.GtfsDB = client | ||
| manager.agenciesMap, manager.routesMap = buildLookupMaps(newStaticData) | ||
| manager.agenciesMap = buildLookupMaps(newStaticData) |
There was a problem hiding this comment.
I think this line can be totally deleted once you rebase.
2 participants
Fixes: #814
Summary
routesMap map[string]*gtfs.Routefrom theManagerstructFindRoute()now delegates toGtfsDB.Queries.GetRoute()filterTripsByAgency,filterVehiclesByAgency,filterAlertsByAgency) are now pure functions no Manager receiver, nostaticMutex. A singlebuildRouteAgencyMap()batch-fetches route --> agency data from DB before filtering runstrips_for_route_handleruses oneGetRouteDB call instead ofFindRoutebuildLookupMapsnow only builds the agency map