add has_transit_tiles and osm_changeset ID to verbose /status response #4062
Conversation
test/loki_service.cc
Outdated
| R"(","tileset_last_modified":0,"available_actions":["status","centroid","expansion","transit_available","trace_attributes","trace_route","isochrone","optimized_route","sources_to_targets","height","route","locate"]})"}, | ||
| {200, | ||
| R"({"version":")" VALHALLA_VERSION | ||
| R"(","tileset_last_modified":0,"available_actions":["status","centroid","expansion","transit_available","trace_attributes","trace_route","isochrone","optimized_route","sources_to_targets","height","route","locate"],"has_tiles":false,"has_admins":false,"has_timezones":false,"has_live_traffic":false,"has_transit_tiles":false,"osm_changeset":0,"bbox":{"features":[],"type":"FeatureCollection"}})"}, |
There was a problem hiding this comment.
oops.. not that small after all.. obviously I only changed this line
There was a problem hiding this comment.
wouldn't show in the test of course..
|
Jep, re-generating the circleci token seems to have worked @kevinkreiser |
| string version = 6; | ||
| uint32 tileset_last_modified = 7; | ||
| repeated string available_actions = 8; |
There was a problem hiding this comment.
these didn't need a oneof actually
There was a problem hiding this comment.
I guess in an earlier implementation they did but 🤷
There was a problem hiding this comment.
Ah no @kevinkreiser they're needed, just as the other ones which are only available via "verbose": true: in the serializer we're using their has_ method to check if we should set it in the output JSON or leave it out. I guess that wouldn't necessarily need to be handled with oneofs, one could also look at the verbose() in the serializer, but that way we'd tightly couple those outputs to verbose() while now we're more flexible and could disallow including them in the JSON based on other logic.
There was a problem hiding this comment.
i think tightly coupling to verbose makes perfect sense in this case but also i dont care that much! we should just upgrade to newer protobuf and forget the idiocy in the earlier versions of proto3, then we can remove oneof and still have the has_ methods
| const bool has_transit_tiles = | ||
| connectivity_map->level_color_exists(TileHierarchy::GetTransitLevel().level); | ||
| status->set_has_transit_tiles(has_transit_tiles); | ||
| } else { | ||
| const static bool has_transit_tiles = !reader->GetTileSet(3).empty(); | ||
| status->set_has_transit_tiles(has_transit_tiles); |
There was a problem hiding this comment.
if we have a connectivity map, great, we'll use the newly added transit level in there via a new tiny function, else we fall back to the much more expensive tile parsing (static so only the first request has to pay that penalty)
src/loki/status_action.cc
Outdated
| @@ -55,15 +57,23 @@ void loki_worker_t::status(Api& request) const { | |||
|
|
|||
| // get _some_ tile | |||
| const static baldr::graph_tile_ptr tile = get_graphtile(reader); | |||
| // TODO: get this from the connectivity_map once transit is implemented there | |||
src/loki/status_action.cc
Outdated
| } | ||
|
|
||
| status->set_has_tiles(static_cast<bool>(tile)); | ||
| status->set_has_admins(tile && tile->header()->admincount() > 0); | ||
| status->set_has_timezones(tile && tile->node(0)->timezone() > 0); | ||
| status->set_has_live_traffic(reader->HasLiveTraffic()); | ||
| status->set_osm_changeset(tile && tile->header()->dataset_id()); |
There was a problem hiding this comment.
this is no good. the result is a boolean
nilsnolde
force-pushed
the
nn-more-status-info
branch
from
214e84d to
f9d7d79
Compare
| } | ||
|
|
||
| status->set_has_tiles(static_cast<bool>(tile)); | ||
| status->set_has_admins(tile && tile->header()->admincount() > 0); | ||
| status->set_has_timezones(tile && tile->node(0)->timezone() > 0); | ||
| status->set_has_live_traffic(reader->HasLiveTraffic()); | ||
| status->set_osm_changeset(tile ? tile->header()->dataset_id() : 0); |
There was a problem hiding this comment.
we set the changeset here, but don't set it in the serializer's output JSON if it was 0
| // a 0 changeset indicates there's none, so don't write in the output | ||
| // TODO: currently this can't be tested as gurka isn't adding changeset IDs to OSM objects (yet) | ||
| if (request.status().has_osm_changeset_case() && request.status().osm_changeset()) | ||
| status_doc.AddMember("osm_changeset", |
There was a problem hiding this comment.
since we only set the changeset when there was one, this code path isn't tested. gurka doesn't add changeset IDs to objects. I left a todo
Related work items: valhalla#4062, valhalla#4071, valhalla#4080, valhalla#4084, valhalla#4087, valhalla#4093, valhalla#4154, valhalla#4159, valhalla#4170, valhalla#4175, valhalla#4179, valhalla#4182, valhalla#4189, valhalla#4204, valhalla#4207, valhalla#4209, valhalla#4227, valhalla#4231, valhalla#4233
fixes #4053
Is that small enough?;)
I'll add a changelog later to see if the CI script works again.