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

fixes #3465 #3466 #3468

Merged
merged 2 commits into from
Dec 21, 2021
Merged

fixes #3465 #3466 #3468

merged 2 commits into from
Dec 21, 2021

Conversation

kevinkreiser
Copy link
Member

@kevinkreiser kevinkreiser commented Dec 21, 2021

what was going wrong?

so when going from proto2 to proto3 we get a new scalar type called bytes. this is useful in languages like java and python which want to make assertions about the type of data that is in a string vs a byte array. in c++ we roll our own so the apis are exactly the same regardless.

we have this concept of tagged values which are arbitrary data which we can attach to any edge via its edge info. the tag usually refers to the tag in osm and the value is any payload we want to attach. the first types of these were strings and were additional names for things like tunnels and bridges. but then we added other stuff like layer which is an integer and pronunciation which is a custom c++ struct and finally also bike share stations which are actually serialized protobufs.

because proto3 doesnt want c++ to send non utf8 binary stuff as a python str or java String and have the person on the other end in those languages get screwed over, they have implemented checks in serialization/deserialization to check that even if we dont differentiate in c++.

how to fix

there were two ways to fix this. the first way would be to not put any non textual things in the tagged value. indeed we are currently only using tunnels for the osrm serializer. so in triplegbuilder we could have filtered the binary ones out. the thing is though that then if you called trace attributes and you wanted all that data you would only get the filtered list. so instead of that i've just changed the type to bytes so we can keep putting all the values in there and the consumer on the other end can know how and whether or not they want to use the data.

Copy link
Member

@nilsnolde nilsnolde left a comment

Choose a reason for hiding this comment

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

ay, what an easy fix.. thanks!

@brad-4k
Copy link

brad-4k commented Dec 23, 2021

When might we see this in the valhalla:latest docker image? Thanks!

@kevinkreiser
Copy link
Member Author

@brad-4k its merged to master so its already there

@brad-4k
Copy link

brad-4k commented Dec 23, 2021

Is there a way to get a version from it to verify? I pulled yesterday and still getting the error. Trying to update (I think) right now:
$ docker pull docker.pkg.github.com/gis-ops/docker-valhalla/valhalla:latest latest: Pulling from gis-ops/docker-valhalla/valhalla Digest: sha256:17c101c48ae1b5fbfc4d1633e6b34ff99bf95e88ad2b24207830b974c526b671 Status: Image is up to date for docker.pkg.github.com/gis-ops/docker-valhalla/valhalla:latest

I still get:

... end raw bytes. [libprotobuf ERROR google/protobuf/wire_format_lite.cc:626] String field 'valhalla.TaggedValue.value' contains invalid UTF-8 data when serializing a protocol buffer. Use the 'bytes' type if you intend to send raw bytes. 2021/12/23 22:13:04.209036 [INFO] Got Odin Request 0 [libprotobuf ERROR google/protobuf/wire_format_lite.cc:626] String field 'valhalla.TaggedValue.value' contains invalid UTF-8 data when parsing a protocol buffer. Use the 'bytes' type if you intend to send raw bytes. 2021/12/23 22:13:04.209405 [ERROR] Failed parsing pbf in Odin::Worker 0 2021/12/23 22:13:04

@kevinkreiser
Copy link
Member Author

kevinkreiser commented Dec 24, 2021

sorry this repository is not connected to the gis-ops one. that is a downstream fork much more convenient-to-use image which builds off of the one our CI pushes, that @nilsnolde manages. im not 100% sure of all of the additions there but i would assume they are the ones that setup service configuration and starting of the service. if you have questions about that please lets keep them over in that repository: https://github.com/gis-ops/docker-valhalla

@brad-4k
Copy link

brad-4k commented Dec 24, 2021

Oh, right! Sorry! In the famous words of Homer Simpson, DOH!

@kevinkreiser
Copy link
Member Author

No worries, an honest mistake!

@nilsnolde
Copy link
Member

we rebuild and push a new image every saturday morning (it's a cron) as latest. that's the periodic reminder to stay away from latest images or master/main code if you depend on it working :)

@kevinkreiser kevinkreiser deleted the kk_pbf3_bytes branch January 5, 2022 03:35
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.

None yet

3 participants