koopjs / koop-output-vector-tiles

Apache License 2.0
17 stars 9 forks source link

No error handling for null tiles. #3

Open reyemtm opened 5 years ago

reyemtm commented 5 years ago

Is this still in development? I am looking at using Koop to add layers into AGOL using the feature server output from static geojson files. This seems like the simplest method for accomplishing this outside of loading the data into AGOL. If so I will add more issues. If not disregard.

If geojsonVT(data).getTile(z, x, y) is null then the plugin crashes as it calls vtpbf using a null tile. A simple fix would be something like the following:

  if (!tile) {
    res.writeHead(204, { 'Access-Control-Allow-Origin': '*' })
    return res.end()
  }
rgwozdz commented 5 years ago

hi @reyemtm - we would definitely like to keep moving forward on this plugin; just looking for contributors. You should submit your fix as PR and I will happily review.

BTW, Koop sets CORS wide-open internally, so I am not sure you need the { 'Access-Control-Allow-Origin': '*' }, but I haven't looked into it.

reyemtm commented 5 years ago

Sounds good. There are a few other updates I have in mind. I have another issue which might be related to the core...response time is slow to get to the output plugin...not sure how to debug that one.

On Wed, May 22, 2019, 12:22 PM Rich Gwozdz notifications@github.com wrote:

hi @reyemtm https://github.com/reyemtm - we would definitely like to keep moving forward on this plugin; just looking for contributors. You should submit your fix as PR and I will happily review.

BTW, Koop sets CORS wide-open internally, so I am not sure you need the { 'Access-Control-Allow-Origin': '*' }, but I haven't looked into it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/koopjs/koop-output-vector-tiles/issues/3?email_source=notifications&email_token=ABQ2HUN7C3UONMV6KZSWEYTPWVXLNA5CNFSM4HOV4PXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV7S62I#issuecomment-494874473, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQ2HUKPKTDMUJQBHMELZBLPWVXLNANCNFSM4HOV4PXA .

rgwozdz commented 5 years ago

What provider are you using?

reyemtm commented 5 years ago

Local file geojson provider. You can check the request received log in the output plugin. I think I am right in that the data file is passed to memory by default?

On Wed, May 22, 2019, 4:19 PM Rich Gwozdz notifications@github.com wrote:

What provider are you using?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/koopjs/koop-output-vector-tiles/issues/3?email_source=notifications&email_token=ABQ2HUPI7BHGNAO5GP64MVLPWWTELA5CNFSM4HOV4PXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWAGY6A#issuecomment-494955640, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQ2HUKYE3JHLTDPXXW5IILPWWTELANCNFSM4HOV4PXA .

rgwozdz commented 5 years ago

Yes, that provider has data cached for 10 seconds (geojson.ttl = 10). After 10 seconds, it will fire getData again and re-read the file.

reyemtm commented 5 years ago

Oh thanks that explains what I am experiencing. I will look at adjusting the cache timeout.

On Wed, May 22, 2019, 4:57 PM Rich Gwozdz notifications@github.com wrote:

Yes, that provider has data cached for 10 seconds (geojson.ttl = 10). After 10 seconds, it will fire getData again and re-read the file.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/koopjs/koop-output-vector-tiles/issues/3?email_source=notifications&email_token=ABQ2HUJ3ITBKUCIMNVZGEF3PWWXSVA5CNFSM4HOV4PXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWAKIYY#issuecomment-494969955, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQ2HUNQA2HEKDPTFKYBVODPWWXSVANCNFSM4HOV4PXA .

reyemtm commented 5 years ago

A couple more thoughts on this cache. Increasing the cache ttl fixed the behavior I was seeing. Any reason to keep this limit low? Is there a fear that keeping the file in cache will lead to high mem usage, or is it that the underlying file could change? Trying to figure out the reasoning so I can better justify my approach.

On Wed, May 22, 2019, 4:57 PM Rich Gwozdz notifications@github.com wrote:

Yes, that provider has data cached for 10 seconds (geojson.ttl = 10). After 10 seconds, it will fire getData again and re-read the file.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/koopjs/koop-output-vector-tiles/issues/3?email_source=notifications&email_token=ABQ2HUJ3ITBKUCIMNVZGEF3PWWXSVA5CNFSM4HOV4PXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWAKIYY#issuecomment-494969955, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQ2HUNQA2HEKDPTFKYBVODPWWXSVANCNFSM4HOV4PXA .

rgwozdz commented 5 years ago

There's no particular reason the ttl is to 10 seconds. I use this provider mainly for debugging and testing koop-core, FeatureServer, and Winnow, so I haven't really scrutinize performance details.

Ideally, the ttl should be set by a provider-namespaced config value, so that it's not hardcoded in the getData function. Feel free to submit a PR on that repo, or maybe I can squeeze in the change sometime soon.