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

Replace endpoint builder with ObjectStore abstraction #36

Merged
merged 1 commit into from
May 28, 2024

Conversation

Randgalt
Copy link
Member

@Randgalt Randgalt commented May 28, 2024

ObjectStore is a better abstraction and will allow support for
various S3-compatible object stores.

Closes #20

@cla-bot cla-bot bot added the cla-signed label May 28, 2024
@Randgalt Randgalt changed the base branch from main to jordanz/switch-to-minio-test-container May 28, 2024 08:55
@Randgalt Randgalt changed the title Support ports in real URI (used when testing, for example) Replace endpoint builder with ObjectStore abstraction May 28, 2024
@Randgalt Randgalt requested review from dain, vagaerg and mosiac1 May 28, 2024 08:56

import jakarta.ws.rs.core.UriBuilder;

import java.net.URI;

@FunctionalInterface
public interface S3EndpointBuilder
public interface ObjectStore
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what to think of the ObjectStore name. This interface only really makes sense for S3-like (though not necessarily S3) endpoints. Calling it ObjectStore IMHO gives the impression that implementations of this interface deal with the quirks of each storage system, yet they would not.

What about S3StoreEndpointBuilder?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm thinking we'll have more functionality in the future besides building endpoints. What about RemoteS3Facade or something like that?

Copy link
Member

Choose a reason for hiding this comment

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

As a follow-up: we've discussed under #20 that we wouldn't support non-S3-like endpoints yet, but if the goal of this current PR is to enforce a bit of separation between the fact we are outwardly S3-like but want to be internally storage agnostic, we could go a bit further and parse the request into a common record.

We could then write the current proxy logic as an implementation of a StorageForwarder interface - wdyt? Happy to create an issue if you'd like

Copy link
Member Author

Choose a reason for hiding this comment

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

Please do

Copy link
Member

Choose a reason for hiding this comment

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

#37 - done

Copy link
Member Author

Choose a reason for hiding this comment

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

@vagaerg So for this PR, what name should we use?

Copy link
Member

Choose a reason for hiding this comment

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

As you prefer - RemoteS3Facade is fine by me, though this isn't a particularly complex example of it :)
S3EndpointBuilder sounded fine to me too

Copy link
Member Author

Choose a reason for hiding this comment

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

done - PTAL

@Randgalt Randgalt requested a review from vagaerg May 28, 2024 13:20
@Randgalt Randgalt force-pushed the jordanz/switch-to-minio-test-container branch from 9d5d4f5 to ba0089c Compare May 28, 2024 13:45
@Randgalt Randgalt force-pushed the jordanz/object-stores branch from 01214a2 to 3752f3a Compare May 28, 2024 13:52
Base automatically changed from jordanz/switch-to-minio-test-container to main May 28, 2024 14:17
@Randgalt Randgalt force-pushed the jordanz/object-stores branch 3 times, most recently from be5eae1 to ee6e3d4 Compare May 28, 2024 14:25
`RemoteS3Facade` is a better abstraction and will allow support for
various S3-compatible object stores.

Closes #20
@Randgalt Randgalt force-pushed the jordanz/object-stores branch from ee6e3d4 to c9a1e83 Compare May 28, 2024 14:29
@Randgalt Randgalt merged commit f71d1d2 into main May 28, 2024
2 checks passed
@Randgalt Randgalt deleted the jordanz/object-stores branch May 28, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Add abstraction for real/target object store
2 participants