FileSystem API Enhancement

[Yohahaha]

Description

FileSystem accept config and as a singleton created at first call of FileHandleGenerator, FileHandleGenerator was hold by HiveConnector and initialize it in ctor.

HiveConnector accept static configs in its ctor, and also accept dynamic(session) configs at runtime after PR #7659. So the issue is what kind of configs should we pass to FileSystem? If FileSystem keep singleton, it need accept HiveConnector's static config for necessary initialization. If FileSystem be per-session instance, we may need move FileHandleGenerator into QueryCtx and use session config to initialize it.

I prefer make FileSystem be per-session instance, it helps to change derived class's behavior at runtime, e.g. accessing S3 with another IAM or apply optimized config without restart.

cc @mbasmanova @majetideepak @zhli1142015

[majetideepak]

@Yohahaha Thanks for opening this issue. The FileSystem API needs an overhaul. There has been a lot of confusion/bugs around getFileSystem API and the optional config. We need a cleaner API to fix this and also allow per-session config.
In the past, I felt having too many file-system instances was bad since each fs client instantiates and consumes resources (threads, buffers, etc). So keeping an instance per session might not be ideal.
My thought is to have a single file-system instance per unique config which includes credentials.
I will be adding some thoughts on the API later today.

[skadilover]

My thought is to have a single file-system instance per unique config which includes credentials.
This design is really nice，

if the scenario where users create a new configuration & FileSystem through the client using SQL will be taken into account， Instead of using configuration files.
@majetideepak

[mbasmanova]

if the scenario where users create a new configuration & FileSystem through the client using SQL

@skadilover Would you share some more details and examples of this use case? Which SQL engine you are working with?

[skadilover]

@mbasmanova

1. SQL engine is self-developed and provides database services to our users as a whole.
2. Functions is similar with doris(just an example we define file config in table properties) : https://doris.apache.org/docs/sql-manual/sql-functions/table-functions/s3?_highlight=s3
   

The key point is that "file system config" is defined in table properties.

[majetideepak]

Inside the HiveConnector, we can make the hiveConfig_ mutable so that we can set/update values from the ConnectorQueryCtx. That should help your use case. ConnectorQueryCtx should have the client values/configs.

[majetideepak]

In Velox, we have the following filesystems: local/Linux filesystem, S3, ABFS, GCS, and HDFS. Some properties across these filesystems are as follows:

1. Some filesystems like S3, HDFS can have multiple instances based on the endpoint, credentials.
2. Some filesystems like HDFS depend on the URI to get the filesystem endpoint.
3. Only the local/Linux filesystem does not require a config.

The current FileSystem APIs are

std::shared_ptr<FileSystem> getFileSystem(std::string_view filePath, std::shared_ptr<const Config> config);
void registerFileSystem(
     std::function<bool(std::string_view)> schemeMatcher,
     std::function<std::shared_ptr<FileSystem>(std::shared_ptr<const Config>, std::string_view)> fileSystemGenerator);

In the current implementation, it is up to the filesystem implementation to create and cache a filesystem instance. GCS, ABFS, LocalFS cache a single instance. This can change in the future. S3, HFDS cache multiple instances.
We have been seeing some bugs with the current API if the config is a nullptr or the implementation does not validate the config across different getFileSystem calls. We must fix this.

I propose the following changes after seeking some inspiration from the PrestoFileSystemCache

1. All invocations of getFileSystem() must specify a non-null config.
2. A new API getLocalFileSystem() will be added to code paths(eg. spilling) where only the local filesystem is expected to be used. No config or filePath will be required here.
3. The filesystem instance caching will be part of the base FileSystem class. A specific filesystem implementation can control the caching via the new API below.
4. A FileSystemFactory class with the API below will be introduced. Each implementation will have a name. This will avoid registering a FileSystemFactory twice and enable removing a FileSystemFactory, which is currently unavailable.

class FileSystemFactory {
 public:
  explicit FileSystemFactory(const char* name) : name_(name) {}

  virtual ~FileSystemFactory() = default;

  // Same as current schemeMatcher.
  virtual bool schemeMatcher(std::string_view filePath) = 0;

  // Same as current fileSystemGenerator.
  virtual std::shared_ptr<FileSystem> createFileSystem(std::string_view filePath, std::shared_ptr<const Config> config) = 0;

  // Create a key based on the input configuration. This could simply be the endpoint or endpoint + credentials.
  virtual std::string createKey(std::string_view filePath, std::shared_ptr<const Config> config) = 0;

  // Return true if the new config requires the filesystem cache entry for a given key to be refreshed, false otherwise. 
  // This API allows a specific cache entry to be replaced.
  // Example: If the credentials are expiring soon, a DB admin could invoke getFileSystem() with the new config.
  virtual bool refresh(std::string_view filePath, std::shared_ptr<const Config> config, std::shared_ptr<const Config> oldConfig)

  const std::string& name() const {
    return name_;
  }

 private:
  const std::string name_;
};


std::shared_ptr<FileSystem> getFileSystem(std::string_view filePath, std::shared_ptr<const Config> config) {
  VELOX_CHECK(config); // check config is not null.
  const auto& registeredFileSystemFactories = fileSystemFactories();
  for (const auto& fileSystemFactory : registeredFileSystemsFactories) {
    if (fileSystemFactory.schemaMatcher(filePath)) {
       auto key = fileSystemFactory.createKey(filePath, config);
       auto fs = fsCache[key];
       if (!fs || fileSystemFactory.refresh(filePath, config, fs->config()) {
           // Create a new filesystem.
           fs =  fileSystemFactory.createFileSystem(filePath, config);
           fsCache[key] = fs;
       }
       return fs;
    }
  }
  VELOX_FAIL("No registered file system matched with file path '{}'", filePath);
}

@Yohahaha The proposed API will allow you to create a file system per session if desired. You can define a new factory with a custom key generator.
CC: @mbasmanova

[Yohahaha]

Thank you for this great proposal!

[acvictor]

Is anyone working on this? I can potentially take this up as I'm looking into supporting storage providers that are set via configs post session creation.

[mbasmanova]

@acvictor

I'm looking into supporting storage providers that are set via configs post session creation.

Where can I learn more about this? Do you have a GitHub issue or a design doc?

[acvictor]

@mbasmanova I've just been thinking about support for other kinds of authentication (at least for ABFS) beyond account keys (very few users directly use keys from what I've seen). In the case of ABFS my idea was to have a blob client factory that has a mapping of account -> blob client based on fs configs, and a single ABFS file system that's registered during initialization. The blob client providers can be registered based on whatever configs are present during initialization but the main design choice then becomes registering blob clients from configs set after session creation. There needs to be some place in the code that can identify change in fs configs and register a new auth provider with minimal config parsing.

[mbasmanova]

@majetideepak Deepak, thank you for putting together a proposal. Overall looks good. I have a question about refresh API.

virtual bool refresh(std::string_view filePath, std::shared_ptr config, std::shared_ptr oldConfig)

FileSystem object is shared and therefore it seems a bit strange that a single user can modify it by calling refresh. Won't this surprise other users? Also, having to pass old config seems inconvenient. Is it needed? Can we remove this parameter?

[mbasmanova]

@majetideepak Deepak, thank you for clarifying. I'm not sure I understand this fully yet though.

// Refresh the filesystem for a given key. Example: Renew credentials for a specific endpoint.

This comment confuses me as it suggest that existing FileSystem object is being modified. Also, it is not clear to me why we need it at all if we already have a key which encodes all the relevant info from path and config. If key changes we create new object. If key is same we return cached object. Am I missing something?

[majetideepak]

This comment confuses me as it suggest that existing FileSystem object is being modified.

@mbasmanova Masha, sorry for the confusion. I fixed the comment now. The existing FileSystem does not change. The existing cache entry for a given key will change.

it is not clear to me why we need it at all if we already have a key which encodes all the relevant info from path and config

As noted in the updated comment, the refresh API allows a specific cache entry to be removed/replaced.
It is possible two configs could map to the same cache key.
Assume a scenario where only the DB admin or a Key Management Service can provide the credentials for a given endpoint (cache key). The DB admin (or service) first creates a cache entry for a given endpoint. Regular users provide the endpoint as the key to access the FS instance. Now if the credentials are about to expire, the DB admin (or service) can refresh the FS instance by providing the new config. New users will pick up the new filesystem instance. Existing users will not be impacted and will continue to use the old FS instance.

Does this clarify? I am happy to elaborate further.

[mbasmanova]

@majetideepak Deepak, thank you for further clarifications. I'm starting to understand. It sounds like key may not encode all the relevant properties from the config, but only so called user properties. The config may also include admin properties that are usually not included in the key and can be applies only use "refresh" API. Is this about right? If so, it is still quite confusing. If we need to distinguish between 2 different kinds of users of the API, maybe we can make it more explicit.

[mbasmanova]

@majetideepak It almost sounds like "admin" API may need to specify additional configs for creating FileSystem objects based on "user" properties.

[majetideepak]

The config may also include admin properties that are usually not included in the key and can be applies only use "refresh" API. Is this about right?

@mbasmanova this is correct!

We cannot separate admin API from user API in Velox since there is no authorization support.
The admin/user is one scenario. The other scenario is where a user brings their credentials and wants to refresh. They could encode the credentials in the key for this scenario, but then you need two types of key generators based on the endpoint.
It is possible that both these scenarios exist in the same application. Say there are two S3 endpoints, one which is admin-controlled and another which is user-controlled.