Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

write multiple PBFs if the protobuf object gets too big #3954

Merged
merged 10 commits into from
Feb 14, 2023
Merged

write multiple PBFs if the protobuf object gets too big #3954

merged 10 commits into from
Feb 14, 2023

Conversation

nilsnolde
Copy link
Member

@nilsnolde nilsnolde commented Feb 7, 2023

We had a small bug where large PBF objects wouldn't be able to get serialized.

Comment on lines 131 to 135
std::string get_tile_path(const std::string tile_dir, const GraphId& tile_id) {
auto file_name = GraphTile::FileSuffix(tile_id);
file_name = file_name.substr(0, file_name.size() - 3) + "pbf";
return tile_dir + filesystem::path::preferred_separator + file_name;
};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pulled this out into a separate function, it's needed in multiple other functions

@nilsnolde
Copy link
Member Author

nilsnolde commented Feb 8, 2023

~~I quickly tried one way of doing it, but I really really don't like it @kevinkreiser . If you have a better idea later, most welcome, otherwise I'll pore over it tmrw to make it nicer. ~~

I guess we could iterate over the trips in the ingest_tiles function and have the write_stop_pairs function only write a single stop pair. Then at least the whole PBF size check would be more in our faces. Looks kinda odd right now..

EDIT: I made it nicer, couldn't stand having it like that. Note, it's failing a test right now, but can only be a small thing

Comment on lines 633 to 635
for (const auto& trip : current.trips) {
dangles =
write_stop_pair(tile, current, trip, feeds(trip), platform_node_ids, uniques) || dangles;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

easier to loop over trips right here..

@@ -33,6 +33,7 @@ config = {
'transit_dir': '/data/valhalla/transit',
'transit_feeds_dir': '/data/valhalla/transit_feeds',
'transit_bounding_box': Optional(str),
'transit_pbf_limit': 20000,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those are representing gtfs trips. let's see what real feeds do. thanks @kevinkreiser for suggesting that.

lock.unlock();
throw std::runtime_error("Couldn't load " + file_name);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first unlock, then throw

Comment on lines +327 to +332
if (std::isdigit(fname.back())) {
// we produce 2 pbf tiles on purpose, where the last one (xx.pbf.0) only has a bunch of stop
// pairs
EXPECT_NE(transit.stop_pairs_size(), 0);
continue;
}
Copy link
Member Author

@nilsnolde nilsnolde Feb 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with mjolnir.transit_pbf_limit: 1 we get two PBFs (one with xx.pbf.0), but the last one only has a few stop pairs, most of the objects are in the first one

* @param file_name name of the file
* @return transit data of the tile with the given GraphId
*/
Transit read_pbf(const baldr::GraphId& id, const std::string& transit_dir, std::string& file_name);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed overload

@nilsnolde
Copy link
Member Author

This should be done for now @kevinkreiser .

  • added a mjolnir.transit_pbf_limit config option to limit the size of trips per feed
  • made convert_transit compatible with extended transit PBF file paths
  • fixed test

kevinkreiser
kevinkreiser previously approved these changes Feb 14, 2023
@nilsnolde nilsnolde merged commit 28889b6 into valhalla:master Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants