allow big GTFS-realtime feeds by increasing protobuf size limit to 2G - #2739
Conversation
|
Hi @ccyrille, the inbuilt limit on protobuf messages is probably intended as a failsafe in case of bad inputs. We should consider raising the limit if it's necessary to parse larger files, but rather than raising it to the absolute maximum, perhaps we should raise to some other reasonable limit. Can you give an example of GTFS-RT files you've encountered that are too big to parse (>64MB)? I would tend to think that a realtime feed bigger than 64MB would be better delivered as streaming / incremental messages, but if someone's only delivering large static files I guess you have to be able to load them. Another point: it seems like the code changes might be in generated or imported code. Is there any way to set this limit via code that is more within the scope of OTP itself rather than generated/imported code? |
|
Hi @abyrd , thanks for your feedback. In fact this hard limit has been considered as somewhat unintentional / deprecated by a Google staff member (see protocolbuffers/protobuf#2228 and protocolbuffers/protobuf#2431) and merged for what would have been protobuf 2.7.1 (unfortunatelly it never shipped, they jumped right away to 3.x which is a big change). I don't know public feeds weighting more than 64MB but our main feed @klaxit (private for now) can reach temporary around 160MB in certain circumstances (far larger than the usual feed but we need to support it) and it is handled pretty well be OTP (no major freeze). In the other hand, I agree that a failsafe could be a nice idea to avoid getting bloated. But for me this shouldn't be done at protocol level (the reason why I had to customize the imported file, there no API to change that from an "external" point of view). Maybe as a real-time updater option ? I don't have much time these days but if you think it's a good idea I will find a way to dig this... Please let me know ! |
abyrd
left a comment
There was a problem hiding this comment.
Based on the linked Google issue this does seem to provide standard behavior.
|
I am merging this with one review as a small self-contained bug fix. |
To be completed by pull request submitter:
To be completed by @opentripplanner/plc: