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

GCS=1 enabling GCS-specific functionality. #10277

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

johngcrowley
Copy link

Problem

  • S3 API mostly works with GCS, except it's missing a bulk delete endpoint implementation and will fail on a NoSuchKey when attempting to re-delete or remove an object that isn't there. S3 behavior is just to warn.

Summary of changes

  • Minimal switch to resolve these two incompatibility issues by setting GCS=1; it now loops over deletion queue vector and throws warning instead of an error on the 404.
  • This fix works in my company's production, self-hosted version of Neon
  • Happy for further discussion, and wanted to see what Neon recommends for further development here

@johngcrowley johngcrowley requested a review from a team as a code owner January 3, 2025 21:22
@github-actions github-actions bot added the external A PR or Issue is created by an external user label Jan 3, 2025
@arpad-m
Copy link
Member

arpad-m commented Jan 4, 2025

FYI we also have #10181 as a proposal. As we haven't decided yet wether we want to support OpenDAL, one can't say whether we want to go with a minimal approach like this one or a larger approach like using OpenDAL / SDK crates (Personally I lean towards this PR).

There is the ability to send bulk deletes as explained here. Of course, implementing those is more involved than just sending single http requests, but http requests do have an overhead, even with http2 and pooling. This shouldn't stop this PR from merging though.

Just out of curiosity, how are you doing the authentication? Which authentication mechanisms work on GCP with the AWS SDK?

@johngcrowley
Copy link
Author

FYI we also have #10181 as a proposal. As we haven't decided yet wether we want to support OpenDAL, one can't say whether we want to go with a minimal approach like this one or a larger approach like using OpenDAL / SDK crates (Personally I lean towards this PR).

There is the ability to send bulk deletes as explained here. Of course, implementing those is more involved than just sending single http requests, but http requests do have an overhead, even with http2 and pooling. This shouldn't stop this PR from merging though.

Just out of curiosity, how are you doing the authentication? Which authentication mechanisms work on GCP with the AWS SDK?

Hey @arpad-m, thank you for taking a look.

We are using HMAC keys and export them as AWS_SECRET_ACCESS_KEY and AWS_ACCESS_KEY_ID, respectively, like regular AWS Neon.

Yes, I am thinking about implementing the GCS API directly with http / reqwest to make use of its bulk delete endpoint, for example, but wasn't sure which route Neon wanted to go. Is that what you're thinking about? I saw this comment that mentioned a potential preference for this route, as opposed to vendor-specific SDKs, but thought the minimal approach was a good starting place.

Eager for more discussion and to be of service here, however I can be.

@erikgrinaker erikgrinaker requested review from arpad-m and removed request for erikgrinaker January 6, 2025 14:03
@erikgrinaker
Copy link
Contributor

@arpad-m I'll be busy with oncall for a bit -- reassigning the review to you since you have more context here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external A PR or Issue is created by an external user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants