Conversation
mrambacher
force-pushed
the
StorageProvider
branch
4 times, most recently
from
8156f76 to
517ddeb
Compare
This change moves the functionality for dealing with S3-like bucket objects into a StorageProvider class. The CloudEnvOptions now has a CloudStorageProvider instance. Additionally, the code was "cleaned up" to allow it to more easily be written as "Configurable" and "Customizable". This basically means that the construction of objects was separated from the configuration of objects was separated from the verification/preparation of objects. By separating these three steps, the objects (AwsEnv, S3StorageProvider, Kinesis and Kafka LogControllers) will be easily adapted to loading from property/INI files.
mrambacher
force-pushed
the
StorageProvider
branch
from
517ddeb to
148c729
Compare
dhruba
left a comment
dhruba
left a comment
There was a problem hiding this comment.
Most of the changes look very good. I would like Igor to take look as well.
| // functionality of another Env. | ||
| class CloudEnvWrapper : public CloudEnvImpl { | ||
|
|
||
| class MockCloudEnv : public CloudEnv { |
There was a problem hiding this comment.
can we keep the classname as CloudEnvWrapper (just to keep compatibility with how a similar class is named in rocksdb, also because this class is not really a ock but rather a wrapper.
There was a problem hiding this comment.
This class is no longer used at all, but I plan to use it later in tests. It is not really a wrapper (it does not contain another CloudEnv). It is a Mock or a Dummy/Test.
I skipped this change for the moment but can revisit it
| virtual Status DoCloudRead(uint64_t offset, size_t n, char* scratch, | ||
| uint64_t* bytes_read) const = 0; | ||
|
|
||
| std::shared_ptr<Logger> info_log_; |
There was a problem hiding this comment.
It is not ideal to have protected members in a public header file. Is there a way to hide these protected member into a xxx_impl.cc file in the src/cloud directory ? (could be a follow up patch)
There was a problem hiding this comment.
I created an Impl class to address these concerns
|
Hi @mrambacher, I'm sorry for the delay in the review. This pull request is much bigger than what would be realistic to review with high quality. Would it be possible to break it down into a couple of steps where each step would be a coherent logical change that would be easier to review? |
|
do we still need this @mrambacher ? |
|
@dhruba I believe this can be closed, as the functionality has been merged in a series of other, smaller PRs. Before I close it, I will double-check to make sure the sum of the effort has been encapsulated in other PRs. |
3 participants
This class moves the code dealing with storing objects to the cloud into a CloudStorageProvider class. The S3 specific code is moved into an S3StorageProvider class. This change will make it easier in the future to support other CloudStorageProviders (like google or Azure).
A future PR will push much of the code that talks to the StorageProvider and LogController from AwsEnv up to the CloudEnvImpl, making the code more re-usable.