Skip to content

Conversation

mosiac1
Copy link
Contributor

@mosiac1 mosiac1 commented Apr 23, 2025

Replace the RemoteS3Facade plugin type with a RemoteS3FacadeFactory that creates RemoteS3Facade instances based on Map<String, String> configs.

Add RemoteS3FacadeManager to create the default RemoteS3Facade from configs in remote-s3-facade.properties.

@cla-bot cla-bot bot added the cla-signed label Apr 23, 2025
@mosiac1 mosiac1 changed the base branch from feat/refactor-remote to main May 7, 2025 08:33
@mosiac1 mosiac1 force-pushed the feat/refactor/remotes-s3-facade-plugins branch from 28068c0 to 0abf14e Compare May 7, 2025 08:39
@mosiac1 mosiac1 requested review from Randgalt and Laonel May 7, 2025 08:44
@Randgalt
Copy link
Member

Why a Map? Why not use a real object via Airlift config?

@mosiac1
Copy link
Contributor Author

mosiac1 commented May 14, 2025

I followed the pattern that Trino uses to create plugins.

I can look into changing it to take a Airlift config, but that would essentially just move the code that does

return new Bootstrap(new DefaultRemoteS3Module())
                .doNotInitializeLogging()
                .quiet()
                .setRequiredConfigurationProperties(configs)
                .initialize()
                .getInstance(RemoteS3Facade.class);

from the RemoteS3FacadeFactory impl to the aws-proxy core.

We would also have to update the TrinoAwsProxyServerPlugin to add a function like

    Collection<Module> remoteS3FacacdeModules();

since we would need to use these to create a new Bootstrap context with only RemoteS3Facade modules and instantiate a RemoteS3Facade from the configs returned by RemoteS3ConnectionProvider

This would have the advantage of removing the need for a separate remote-s3-facade.properties config file.

WDYT?

@Randgalt
Copy link
Member

I can tell you from experience that the raw HashMap config for connectors is a major headache. Also, Trino plugins/connectors are usually their own context, with their own classloader, etc. We don't want to emulate that.

@Override
public RemoteS3Facade create(Map<String, String> configs)
{
return new Bootstrap(new DefaultRemoteS3Module())
Copy link
Member

Choose a reason for hiding this comment

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

I have serious concerns with starting a new Bootstrap with this library. I feel we're getting into the weeds. Maybe you can explain the need for this offline.

Copy link
Member

Choose a reason for hiding this comment

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

Note: log is unused

Update RemoteS3Facade default bindings to not set the optional default but the value.
RemoteS3Connection no longer has a function to get a Map of configs, but gets a function to get an instantiated RemoteS3Facade.
Add a StaticRemoteS3Connection record to replace the previous one.
@mosiac1 mosiac1 force-pushed the feat/refactor/remotes-s3-facade-plugins branch from cf4c81a to 072cce2 Compare May 14, 2025 15:00
@mosiac1 mosiac1 merged commit f2353a5 into main May 14, 2025
2 checks passed
@mosiac1 mosiac1 deleted the feat/refactor/remotes-s3-facade-plugins branch May 14, 2025 15:06
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.

2 participants