Clarify CloudFileSystem API. #313
Conversation
Splitting the CloudFileSystem class into a pure interface class and a separate class that contains static initialization and factory methods (CloudFileSystemEnv). This will make it possible to provide other implementations of CloudFileSystem wihtout having to wrangle with static functions and options.
| // constructed, and remain active (Aws::ShutdownAPI() not called) as long as any | ||
| // CloudFileSystem objects exist. | ||
| class CloudFileSystemEnv : public CloudFileSystem { | ||
| protected: |
There was a problem hiding this comment.
curious why do we keep this separate class instead of moving all these static functions and member variables to CloudFileSystemImpl and simply removing this CloudFileSystemEnv class?
nvm, seems we access these static methods in rockset codebase.
Would it be better to move all these member variables and member functions to CloudFileSystemImpl and only keep these static functions in CloudFileSystemEnv so that CloudFileSystemEnv doesn't have to inherit the interface CloudFileSystem?
There was a problem hiding this comment.
I am not sure. I kind of assumed there was a reason for these methods to be in CloudFileSystem in the first place. Maybe somebody didn't want to reveal the fact that ColoudFileSystemImpl exists to the client code or didn't want to force them to add another include.
There is some logic to the current state. CloudFileSystem defines the API, CloudFileSystemEnv has various static initialization methods. Theoretically it could even be made separate from the CloudFileSystem class hierarchy altogether. Finally, CloudFileSystemImp implements the CloudFileSystem and FileSystem APIs.
That being said, I am not opposed to moving everything to CloudFileSystemImpl if that is the consensus.
There was a problem hiding this comment.
Theoretically it could even be made separate from the CloudFileSystem class hierarchy altogether. Finally, CloudFileSystemImp implements the CloudFileSystem and FileSystem APIs
Like this idea.
|
Sorry Dmitri, would you mind waiting for SYS-6913 to be done before landing this? |
dpetrov4
force-pushed
the
dmitri_cloudfs
branch
from
b2bf52d to
45be442
Compare
|
I have updated the code to detach CloudFileSystemEnv from the CloudfileSystem->CloudFileSystemImpl hierarchy. Will wait for @igorcanadi to "do his thing" before merging. |
Splitting the CloudFileSystem class into a pure interface class
and a separate class that contains static initialization and factory
methods (CloudFileSystemEnv).
This will make it possible to provide other implementations of CloudFileSystem
wihtout having to wrangle with static functions and options.