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

Support Azure reload in case the file is changing #177

Closed
wants to merge 6 commits into from

Conversation

akhenakh
Copy link

@akhenakh akhenakh commented Aug 8, 2024

Using the same logic as the S3 adapter, here is an Azure adapter that supports ETag and If-Match.

  • separated the code from the s3 adapter

issue #176

@akhenakh akhenakh marked this pull request as ready for review August 9, 2024 18:53
@akhenakh
Copy link
Author

akhenakh commented Aug 9, 2024

confirmed working on azure storage cc @bdon

return ba.Bucket.Close()
}

type AzureBucketAdapter struct {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to centralize this logic in the existing Bucket Adapter instead of creating separate implementations for each of (Azure, S3, etc)? That will be more maintainable in the long term and takes better advantage of gocloud.

Copy link
Author

@akhenakh akhenakh Aug 19, 2024

Choose a reason for hiding this comment

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

I've tried that in this updated version, there is nothing left in a "BaseBucketAdapter" cause everything we do here is using the provider specific APIs (As()), so it's not generic anymore.

The only common left is now in handleReaderResponse()

I think I almost prefer the explicit version with both adapters and a few repetitions,

@bdon
Copy link
Member

bdon commented Sep 6, 2024

I tested out an alternate approach that centralizes all the logic into NewRangeReaderEtag here. Do you think this would work for your use case?

https://github.com/protomaps/go-pmtiles/blob/azure-gcp-etags/pmtiles/bucket.go#L225

I also added support for Google Cloud, which uses integer generation IDs instead of string ETags.

I think this approach takes better advantage of gocloud instead of requiring different concrete implementations, though it does make the single code path quite messy - this does get us to covering all of the cloud storage systems (s3, azblob, gs) so it shouldn't be more complicated unless some new platform emerges and is added.

Btw, this seems to play well with the new Azure Container Apps deployment option I documented here: https://docs.protomaps.com/deploy/azure with Service Connector the connection string-based authentication to Blob Storage works with some small changes.

Thanks for the PR and being patient with this.

bdon added a commit that referenced this pull request Sep 9, 2024
* NewRangeReaderEtag for gocloud buckets does provider-specific operations for set/get etag and error.
* use generation ID as string etag for Google Cloud.
bdon added a commit that referenced this pull request Sep 9, 2024
* NewRangeReaderEtag for gocloud buckets does provider-specific operations for set/get etag and error.
* use generation ID as string etag for Google Cloud.
@bdon
Copy link
Member

bdon commented Sep 9, 2024

Implemented a different way in #183

@bdon bdon closed this Sep 9, 2024
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