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

Add NewServerWithBucket and NewServerWithFS methods #47

Merged
merged 17 commits into from
Oct 16, 2023

Conversation

thisisaaronland
Copy link
Contributor

This PR add two new methods:

  • NewServerWithBucket instantiates a pmtiles.Server instance from a pre-existing gocloud.dev/blob.Bucket instance. Under the hood the NewServer method has been updated to call this after it has created a bucket from a PMTiles path URI.
  • NewServerWithFS instantiates a new in-memory gocloud.dev/blob.Bucket instance and populates it with the contents of a fs.FS instance. Once created it invokes the NewServerWithBucket method.

These changes make it possible to bundle (and use) a PMTiles database as an embed -able asset in a Go program. While this probably doesn't make sense for very large databases it can be useful for smaller databases.

Additionally, this PR:

  • Updates all vendor deps to their current versions
  • Cleans up some errors reported by go vet

@thisisaaronland
Copy link
Contributor Author

@bdon Once I get this back in sync with the latest release is this a patch you are interested in?

@bdon
Copy link
Member

bdon commented Sep 22, 2023

@thisisaaronland thanks for checking in - I have been cowardly avoiding anything related to #46 and #20, because other projects have been higher priority, and exposing a developer facing API in this project needs to come after we have a sufficient core of functionality done - I just added extract and verify.

embed -able asset in a Go program. While this probably doesn't make sense for very large databases it can be useful for smaller databases.

This seems interesting to expose through a programmatic API, but the code that does the embedding needs to live outside of this library - all of the operations in here are intentionally designed to work for any size of archive, up to the planet.

It looks like this works by using fs and then copying it into a in-memory mem: bucket. Can this be accomplished via file:/// bucket type instead that's built in? I also had to add a Bucket abstraction over the CDK one https://github.com/protomaps/go-pmtiles/blob/main/pmtiles/bucket.go#L16 in order to support public HTTP pmtiles URLs that do not let you write into them. If we need to move away from file:// buckets I would prefer to stay within that abstraction instead of special-casing code inside HTTP server parts.

In general, I do want a more modular API for doing what you said - provide a bucket and let you serve it from an embedded program. I don't want to extend the server too far in functionality, though; I'm working on a Caddy plugin https://github.com/orgs/protomaps/discussions/1 that will accomplish more HTTP and header-ish operations.

Finally, I will add a CI task that runs go fmt to make PRs cleaner.

bdon added a commit that referenced this pull request Sep 22, 2023
bdon added a commit that referenced this pull request Sep 22, 2023
* add CI for checking formatting

* go fmt [#47]
@bdon
Copy link
Member

bdon commented Sep 22, 2023

ad2801a added a GitHub action that fails if the code is not formatted with go fmt, so that should clean up the noise on PRs.

@thisisaaronland
Copy link
Contributor Author

It sounds like the easiest thing to do would be to change the signature of NewServerWithFS function to accept a blob URI specifying where a fs.FS instance would be copied to before use. As mentioned the point is not necessarily to embed a multi-GB pmtiles file with a Go binary (and then load it in to memory) but anything less than 100MB seems do-able these days.

That way the function would simply call blob.NewBucket(ctx, user_supplied_bucket_uri) and it would be up to a user to ensure that any relevant blob packages not already loaded by the go-pmtiles package are imported in their own code (for example memblob:// or any other custom blob implementation).

Likewise NewServerWithBucket provides an avenue for people to use the go-pmtiles server code with custom blob implementations (meaning it's up to them to import the relevant packages in the code that uses pmtiles/server.go).

A recent-ish addition to all of this is that blob.Bucket has been updated to implement io/fs.FS and io/fs.SubFS although I have not tried to use this yet:

google/go-cloud#3272
https://github.com/google/go-cloud/blob/master/blob/blob_fs.go

I am not suggesting that go-pmtiles, or at least the server piece which is read-only (?) like fs.FS, be wholesale refactored to use fs.FS. I am just pointing out that it exists.

@thisisaaronland
Copy link
Contributor Author

Related blob.Bucket implements a range reader by default now:

https://pkg.go.dev/gocloud.dev/blob?#Bucket.NewRangeReader

@bdon
Copy link
Member

bdon commented Oct 2, 2023

Can you rebase this off main to pick up the formatting changes?

If I just split the NewServer function into 2 where you can provide your own bucket, is that sufficient?

@thisisaaronland
Copy link
Contributor Author

In the course of getting this to work in another branch, I am no longer certain this PR makes much sense. Specifically, this branch:

https://github.com/protomaps/go-pmtiles/compare/main...sfomuseum:go-pmtiles:fs-bucket-2?expand=1

Which is used here:

https://github.com/sfomuseum/go-sfomuseum-pmtiles/blob/static/cmd/server-static/main.go
https://github.com/sfomuseum/go-sfomuseum-pmtiles/blob/static/application/server/server.go#L62-L78
https://github.com/sfomuseum/go-sfomuseum-pmtiles/tree/static/static

The problem stems from the use/need for a PMTiles-specific Bucket struct rather than blob.Bucket. If the goal is to read a PMTiles database bundled as a io.FS asset in to, and serve from, memory it's not possible to because the pmtiles.Bucket struct has no equivalent of blob.NewWriter for writing data.

Even if a bundled io.FS asset is written to mem:// that memory is not shared when the code finally returns a new pmtiles.Bucket address:

https://github.com/sfomuseum/go-pmtiles/blob/fs-bucket-2/pmtiles/server.go#L147-L153

I suppose there is still some use to the methods in that it would allow you to bundle small-ish maps inside a Lambda function without the need to configure a separate S3 bucket for the tile data itself (writing the data to the functions /tmp directory and assuming that the function and data will be cached for heavy use).

I suppose just having a NewServerWithBucket method (and not a NewServerWithFS method) as part of the go-pmtiles package would still be useful, though.

https://github.com/sfomuseum/go-pmtiles/blob/fs-bucket-2/pmtiles/server.go#L156

Thoughts?

@bdon
Copy link
Member

bdon commented Oct 13, 2023

Even if a bundled io.FS asset is written to mem:// that memory is not shared when the code finally returns a new pmtiles.Bucket address:

Why can't you write to your own mem bucket, hold the pointer, and create a pmtiles Bucket from it?

@thisisaaronland
Copy link
Contributor Author

Yes, that works too:

https://github.com/sfomuseum/go-sfomuseum-pmtiles/blob/static/bucket.go

It does suggest that it would/might be useful to extend that package to implement (wrap) all of the blob.Bucket methods but that's a separate issue.

It's also another argument for not including a NewServerWithFS method in go-pmtiles itself since that can be implemented elsewhere (and because there are still some issues about how/where to close Bucket instances, as written):

https://github.com/sfomuseum/go-sfomuseum-pmtiles/blob/static/server.go

@thisisaaronland
Copy link
Contributor Author

PR has been updated to remove NewBucketWithFS method and simple expose a NewServerWithBucket method. fs.FS is left as an exercise to the reader for the time being though a working example can be found here:

https://github.com/sfomuseum/go-sfomuseum-pmtiles/blob/static/application/server/server.go#L99-L127

https://github.com/sfomuseum/go-sfomuseum-pmtiles/blob/static/bucket/fs.go
https://github.com/sfomuseum/go-sfomuseum-pmtiles/blob/static/bucket/gocloud.go

@bdon bdon merged commit 87f572f into protomaps:main Oct 16, 2023
@bdon
Copy link
Member

bdon commented Oct 16, 2023

Thanks, do you need me to do a minor release (1.10.1)?

@thisisaaronland
Copy link
Contributor Author

Yes, please. Thanks,

@bdon
Copy link
Member

bdon commented Oct 18, 2023

v1.10.4 is tagged with new binaries on releases; I also changed the notarization code to the new Apple tool so let me know if there's any issues with the CLI on Mac.

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.

2 participants