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

Merge mvt_provider and providers into single providers section in config. #688

Closed
gdey opened this issue May 26, 2020 · 6 comments · Fixed by #700
Closed

Merge mvt_provider and providers into single providers section in config. #688

gdey opened this issue May 26, 2020 · 6 comments · Fixed by #700

Comments

@gdey
Copy link
Member

gdey commented May 26, 2020

This is a copy of go-spatial/tegola-postgis#4

with #687 we will have the mvt_provider experiment in the mainline, and so should track currently known issues with it.

Should we remove mvt_provider, and have the provider register as an MVT type provider.

i.e.

[[providers]]
name = "test_postgis"       # provider name is referenced from map layers (required)
type = "mvt_postgis"            # the type of data provider must be "postgis" for this data provider (required)
host = "localhost"          # PostGIS database host (required)
port = 5432                 # PostGIS database port (required)
database = "tegola"         # PostGIS database name (required)
user = "tegola"             # PostGIS database user (required)
password = ""               # PostGIS database password (required)

There are some things to note about this approach.

  1. The provider needs to state that it's an MVT provider (this could be done by look to see if it supports the mvt_provider interface)

  2. Documentation can be confusing as mvt providers can not conflate with other providers.

Errors

If a map has an mvt provider, it can be the only one and the system will error during configuration stating that the mvt_provider can be the only provider.

@gdey
Copy link
Member Author

gdey commented May 26, 2020

@ARolek left this comment:

I think it would be ideal if we could just use the type property and not need to introduce a new top-level provider concept. It would also get rid of the mvt_ prefix that needs to be added to map layers.

Regarding conflation, we can't conflate at this time, but we have talked about ways to make conflating possible so it's not entirely a blocker long term.

@ARolek
Copy link
Member

ARolek commented May 26, 2020

Rather than type=postgis_mvt I was thinking type=mvt_postgis.

@ear7h
Copy link
Contributor

ear7h commented Jun 16, 2020

recap

The current implementation has the restriction that a [[map]] can only use 1 mvt_provider and no other providers. This is because, tegola does not decode the incoming mvt tile, thus cannot insert new geometries from other providers.

a proposed solution

A proposed solution is to register all providers with [[providers]] (with mvt providers having a type=mvt_*). This solution removes the need for a "mvt_" prefix since all providers (mvt and non-mvt) must have a unique name. This is also representative of the trajectory of this feature: at some point in the future we would like to decode the mvt (say, from another vector tile server) insert geometries that are stored locally, and send that as one mvt tile.

One complexity of this approach will be dealing with the two different interfaces in this single [[providers]] list.

my proposal

Something that would better represent the way things work right now would be to add a mvt_provider boolean or string field for [[map]]. This field indicates if, or which, mvt provider is allowed for the map. This would remove the need for a "mvt_" prefix because "mvt-ness" of the [[map]] is declared explicitly, thus there is no ambiguity in the provider_layer field. I think this approach more clearly communicates the current limitations of the mvt providers.

@ARolek
Copy link
Member

ARolek commented Jun 16, 2020

@ear7h Thanks for writing this up. Why would we need mvt_provider boolen in the config if we could just add the check/error reporting in the config parsing step? I admit the logic kind of a pain to lookup (If a map has a provider layer with a type containing an mvt_* prefix, then it can't contain any other provider), but it would allow us to eventually lift that check when we support conflating mvt providers with other providers.

@ear7h
Copy link
Contributor

ear7h commented Jun 16, 2020

@ARolek firstly, my proposal would be temporary. Once tile conflation is implemented, putting all providers in the same section would be ideal.

The boolean config is not functionally necessary. Personally, I'm not a fan of software that tries to fix or be smart about my configuration (ex. elastic search). In my proposal, keeping the mvt and feature providers in separate namespaces means there can be two providers with same name (and referrenced with the same name, no added prefixes), so thw config option disambiguates between them. Alternatively, we can add a [[mvt_maps]] section in the config that can only use mvt providers.

This might be overly complicated for something temporary and that's okay, I'm not super invested in the idea

@ARolek
Copy link
Member

ARolek commented Jun 24, 2020

@superbug1000 has been testing tegola-postgis and has had a chance to try out the config structure. I asked them if they had an opinion on this issue in #696 and this was their input:

I think it makes sense to have just providers and using the type to specify, well, the type. I don't know too much about the project to think about the potential implications though.

@gdey gdey changed the title Should we merge mvt_provider and proviers config options. Merge mvt_provider and providers into single providers section in config. Jul 5, 2020
@gdey gdey closed this as completed in #700 Aug 18, 2020
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 a pull request may close this issue.

3 participants