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

de-singleton tile extract in graphreader #3281

Merged
merged 49 commits into from
Oct 27, 2021

Conversation

nilsnolde
Copy link
Member

@nilsnolde nilsnolde commented Aug 23, 2021

fixes #3117

I started with this on the side, working on it here and there. to re-iterate: we want to be able to load other tile extracts while a consumer application is running (e.g. valhalla_service). in #3117 @kevinkreiser proposed to use a fixed-width index file storing the tile_id and byte offset in the final tar file which the graphreader can read first (if available) to really fast initialize the tile_extract_t (faster because it does not have to scan through the entire tar file thanks to the index file).

tasks:

  • set up python script
  • tests python (needs some refactoring)
  • adapt graphreader to read the index file first if present
  • tests graphreader
  • some open questions:
    • this (currently) does not work for existing extracts, so far it only builds an extract from mjolnir.tile_dir. is that fine with you? we could also add that ability and it sorta was what @kevinkreiser initially suggested -> not necessary IMO
    • where to add this functionality: python bindings for sure IMO, what about valhalla_service? -> subsequent PRs

@kevinkreiser I didn't do much yet on the c++ side, will comment a bit on the files, a few pointers would be cool:)

example commandline: python scripts/valhalla_build_extract --config site/valhalla.json

src/baldr/graphreader.cc Outdated Show resolved Hide resolved
src/baldr/graphreader.cc Outdated Show resolved Hide resolved
src/baldr/graphreader.cc Outdated Show resolved Hide resolved
src/baldr/graphreader.cc Outdated Show resolved Hide resolved
src/baldr/graphreader.cc Outdated Show resolved Hide resolved
@nilsnolde
Copy link
Member Author

nilsnolde commented Oct 21, 2021

this is as good as done @kevinkreiser , just a tiny pointer issue in graphreader.cc, maybe you can suggest the right way?

we also should document this in the docs I think. where would be the best place? small paragraph in the README? added a command to the readme

…ve different tar file sizes with predicted traffic
…so we don't get a race condition with the test which actually creates the tar
@nilsnolde
Copy link
Member Author

I removed setting the tile_extract from the base test config again. as kevin pointed rightfully out, it'd create a race with other tests accessing the utrecht tiles

@kevinkreiser
Copy link
Member

@nilsnolde ill review this today, might be tonight by the time i have time but ill get to it. thanks for hanging in there!

@kevinkreiser
Copy link
Member

ok i made a couple requests for minor changes otherwise looks good!

@nilsnolde
Copy link
Member Author

thanks @kevinkreiser for final review. I changed all the things you requested:

  • sort files in the tar
  • left some todos

finally done 🥳 next up: integrate the ability to reload tiles to python bindings (what else?)

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.

De-Singleton tile_extract_t to Allow for Config (tileset) Reloading
2 participants