Skip to content

Commit b2bf52d

Browse files
dpetrov4dpetrov4
committed
Detaching CloudFileSystemEnv from CloudFileSystem class hierarchy.
1 parent a5e883e commit b2bf52d

File tree

6 files changed

+83
-101
lines changed

6 files changed

+83
-101
lines changed

‎cloud/aws/aws_file_system.cc

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,22 @@
11
// Copyright (c) 2016-present, Rockset, Inc. All rights reserved.
22
//
3+
#include "rocksdb/cloud/cloud_file_system.h"
34
#ifndef ROCKSDB_LITE
4-
#include "cloud/aws/aws_file_system.h"
5-
65
#include <chrono>
76
#include <cinttypes>
87
#include <fstream>
98
#include <iostream>
109
#include <memory>
1110
#include <set>
1211

12+
#include "cloud/aws/aws_file_system.h"
1313
#include "cloud/cloud_log_controller_impl.h"
1414
#include "cloud/cloud_scheduler.h"
15-
#include "rocksdb/cloud/cloud_storage_provider_impl.h"
1615
#include "cloud/filename.h"
1716
#include "port/port.h"
1817
#include "rocksdb/cloud/cloud_log_controller.h"
1918
#include "rocksdb/cloud/cloud_storage_provider.h"
19+
#include "rocksdb/cloud/cloud_storage_provider_impl.h"
2020
#include "rocksdb/env.h"
2121
#include "rocksdb/status.h"
2222
#include "rocksdb/utilities/object_registry.h"
@@ -175,8 +175,7 @@ Status AwsCloudAccessCredentials::GetCredentialsProvider(
175175
AwsFileSystem::AwsFileSystem(const std::shared_ptr<FileSystem>& underlying_fs,
176176
const CloudFileSystemOptions& _cloud_fs_options,
177177
const std::shared_ptr<Logger>& info_log)
178-
: CloudFileSystemImpl(_cloud_fs_options, underlying_fs, info_log) {
179-
}
178+
: CloudFileSystemImpl(_cloud_fs_options, underlying_fs, info_log) {}
180179

181180
// If you do not specify a region, then S3 buckets are created in the
182181
// standard-region which might not satisfy read-your-own-writes. So,
@@ -223,8 +222,8 @@ Status AwsFileSystem::NewAwsFileSystem(
223222
}
224223
std::unique_ptr<AwsFileSystem> afs(
225224
new AwsFileSystem(fs, cloud_options, info_log));
226-
227-
auto env = afs->NewCompositeEnvFromThis(Env::Default());
225+
auto env =
226+
CloudFileSystemEnv::NewCompositeEnvFromFs(afs.get(), Env::Default());
228227
ConfigOptions config_options;
229228
config_options.env = env.get();
230229
status = afs->PrepareOptions(config_options);

‎cloud/cloud_file_system.cc

Lines changed: 8 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -417,18 +417,6 @@ Status CloudFileSystemOptions::Serialize(const ConfigOptions& config_options,
417417
reinterpret_cast<const char*>(this), value);
418418
}
419419

420-
CloudFileSystemEnv::CloudFileSystemEnv(const CloudFileSystemOptions& options,
421-
const std::shared_ptr<FileSystem>& base,
422-
const std::shared_ptr<Logger>& logger)
423-
: cloud_fs_options(options), base_fs_(base), info_log_(logger) {
424-
RegisterOptions(&cloud_fs_options, &cloud_fs_option_type_info);
425-
}
426-
427-
CloudFileSystemEnv::~CloudFileSystemEnv() {
428-
cloud_fs_options.cloud_log_controller.reset();
429-
cloud_fs_options.storage_provider.reset();
430-
}
431-
432420
Status CloudFileSystemEnv::NewAwsFileSystem(
433421
const std::shared_ptr<FileSystem>& base_fs,
434422
const std::string& src_cloud_bucket, const std::string& src_cloud_object,
@@ -492,12 +480,13 @@ void CloudFileSystemEnv::RegisterCloudObjects(const std::string& arg) {
492480
});
493481
}
494482

495-
std::unique_ptr<Env> CloudFileSystemEnv::NewCompositeEnvFromThis(Env* env) {
483+
std::unique_ptr<Env> CloudFileSystemEnv::NewCompositeEnvFromFs(FileSystem* fs,
484+
Env* env) {
496485
// We need a shared_ptr<FileSystem> pointing to "this", to initialize the
497486
// env wrapper, but we don't want that shared_ptr to own the lifecycle for
498487
// "this". Creating a shared_ptr with a custom no-op deleter instead.
499-
std::shared_ptr<FileSystem> fs(this, [](auto* /*p*/) { /*noop*/ });
500-
return std::make_unique<CompositeEnvWrapper>(env, fs);
488+
std::shared_ptr<FileSystem> fsPtr(fs, [](auto* /*p*/) { /*noop*/ });
489+
return std::make_unique<CompositeEnvWrapper>(env, fsPtr);
501490
}
502491

503492
Status CloudFileSystemEnv::CreateFromString(
@@ -529,13 +518,13 @@ Status CloudFileSystemEnv::CreateFromString(
529518
copy.invoke_prepare_options = false; // Prepare here, not there
530519
s = ObjectRegistry::NewInstance()->NewUniqueObject<FileSystem>(id, &fs);
531520
if (s.ok()) {
532-
auto* cfs = dynamic_cast<CloudFileSystemEnv*>(fs.get());
521+
auto* cfs = dynamic_cast<CloudFileSystemImpl*>(fs.get());
533522
assert(cfs);
534523
if (!options.empty()) {
535524
s = cfs->ConfigureFromMap(copy, options);
536525
}
537526
if (s.ok() && config_options.invoke_prepare_options) {
538-
auto env = cfs->NewCompositeEnvFromThis(copy.env);
527+
auto env = NewCompositeEnvFromFs(cfs, copy.env);
539528
copy.invoke_prepare_options = config_options.invoke_prepare_options;
540529
copy.env = env.get();
541530
s = cfs->PrepareOptions(copy);
@@ -583,15 +572,15 @@ Status CloudFileSystemEnv::CreateFromString(
583572
copy.invoke_prepare_options = false; // Prepare here, not there
584573
s = ObjectRegistry::NewInstance()->NewUniqueObject<FileSystem>(id, &fs);
585574
if (s.ok()) {
586-
auto* cfs = dynamic_cast<CloudFileSystemEnv*>(fs.get());
575+
auto* cfs = dynamic_cast<CloudFileSystemImpl*>(fs.get());
587576
assert(cfs);
588577
auto copts = cfs->GetOptions<CloudFileSystemOptions>();
589578
*copts = cloud_options;
590579
if (!options.empty()) {
591580
s = cfs->ConfigureFromMap(copy, options);
592581
}
593582
if (s.ok() && config_options.invoke_prepare_options) {
594-
auto env = cfs->NewCompositeEnvFromThis(copy.env);
583+
auto env = NewCompositeEnvFromFs(cfs, copy.env);
595584
copy.invoke_prepare_options = config_options.invoke_prepare_options;
596585
copy.env = env.get();
597586
s = cfs->PrepareOptions(copy);

‎cloud/cloud_file_system_impl.cc

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,11 @@ namespace ROCKSDB_NAMESPACE {
3232

3333
CloudFileSystemImpl::CloudFileSystemImpl(
3434
const CloudFileSystemOptions& opts, const std::shared_ptr<FileSystem>& base,
35-
const std::shared_ptr<Logger>& l)
36-
: CloudFileSystemEnv(opts, base, l), purger_is_running_(true) {
35+
const std::shared_ptr<Logger>& logger)
36+
: info_log_(logger),
37+
cloud_fs_options(opts),
38+
base_fs_(base),
39+
purger_is_running_(true) {
3740
if (opts.cloud_file_deletion_delay) {
3841
cloud_file_deletion_scheduler_ = CloudFileDeletionScheduler::Create(
3942
CloudScheduler::Get(), *opts.cloud_file_deletion_delay);
@@ -45,6 +48,8 @@ CloudFileSystemImpl::~CloudFileSystemImpl() {
4548
cloud_fs_options.cloud_log_controller->StopTailingStream();
4649
}
4750
StopPurger();
51+
cloud_fs_options.cloud_log_controller.reset();
52+
cloud_fs_options.storage_provider.reset();
4853
}
4954

5055
IOStatus CloudFileSystemImpl::ExistsCloudObject(const std::string& fname) {

‎cloud/cloud_file_system_test.cc

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,9 @@
33
#include "rocksdb/cloud/cloud_file_system.h"
44

55
#include "cloud/cloud_log_controller_impl.h"
6-
#include "rocksdb/cloud/cloud_storage_provider_impl.h"
76
#include "rocksdb/cloud/cloud_log_controller.h"
87
#include "rocksdb/cloud/cloud_storage_provider.h"
8+
#include "rocksdb/cloud/cloud_storage_provider_impl.h"
99
#include "rocksdb/convenience.h"
1010
#include "rocksdb/env.h"
1111
#include "test_util/testharness.h"
@@ -214,8 +214,8 @@ TEST(CloudFileSystemTest, DISABLED_ConfigureKinesisController) {
214214
ASSERT_OK(CloudFileSystem::CreateFromString(
215215
config_options, "id=aws; controller=kinesis; TEST=dbcloud:/test", &cfs));
216216
ASSERT_STREQ(cfs->Name(), "aws");
217-
ASSERT_NE(cfs->GetLogController(), nullptr);
218-
ASSERT_STREQ(cfs->GetLogController()->Name(),
217+
ASSERT_NE(cfs->GetCloudFileSystemOptions().cloud_log_controller, nullptr);
218+
ASSERT_STREQ(cfs->GetCloudFileSystemOptions().cloud_log_controller->Name(),
219219
CloudLogControllerImpl::kKinesis());
220220
#endif
221221
}
@@ -229,8 +229,8 @@ TEST(CloudFileSystemTest, ConfigureKafkaController) {
229229
#ifdef USE_KAFKA
230230
ASSERT_OK(s);
231231
ASSERT_NE(cfs, nullptr);
232-
ASSERT_NE(cfs->GetLogController(), nullptr);
233-
ASSERT_STREQ(cfs->GetLogController()->Name(),
232+
ASSERT_NE(cfs->GetCloudFileSystemOptions().cloud_log_controller, nullptr);
233+
ASSERT_STREQ(cfs->GetCloudFileSystemOptions().cloud_log_controller->Name(),
234234
CloudLogControllerImpl::kKafka());
235235
#else
236236
ASSERT_NOK(s);
@@ -244,4 +244,3 @@ int main(int argc, char** argv) {
244244
::testing::InitGoogleTest(&argc, argv);
245245
return RUN_ALL_TESTS();
246246
}
247-

‎include/rocksdb/cloud/cloud_file_system.h

Lines changed: 4 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -616,29 +616,18 @@ class CloudFileSystem : public FileSystem {
616616
};
617617

618618
//
619-
// The Cloud file system
619+
// The Cloud File System initialization/construction.
620620
//
621621
// NOTE: The AWS SDK must be initialized before the CloudFileSystem is
622622
// constructed, and remain active (Aws::ShutdownAPI() not called) as long as any
623623
// CloudFileSystem objects exist.
624-
class CloudFileSystemEnv : public CloudFileSystem {
625-
protected:
626-
CloudFileSystemOptions cloud_fs_options;
627-
std::shared_ptr<FileSystem> base_fs_; // The underlying file system
628-
624+
class CloudFileSystemEnv {
625+
public:
629626
// Creates a new CompositeEnv from "env" and "this".
630627
// The returned Env must not outlive "this"
631-
std::unique_ptr<Env> NewCompositeEnvFromThis(Env* env);
632-
633-
CloudFileSystemEnv(const CloudFileSystemOptions& options,
634-
const std::shared_ptr<FileSystem>& base,
635-
const std::shared_ptr<Logger>& logger);
628+
static std::unique_ptr<Env> NewCompositeEnvFromFs(FileSystem* fs, Env* env);
636629

637630
public:
638-
mutable std::shared_ptr<Logger> info_log_; // informational messages
639-
640-
virtual ~CloudFileSystemEnv();
641-
642631
static void RegisterCloudObjects(const std::string& mode = "");
643632
static Status CreateFromString(const ConfigOptions& config_options,
644633
const std::string& id,
@@ -648,56 +637,6 @@ class CloudFileSystemEnv : public CloudFileSystem {
648637
const CloudFileSystemOptions& cloud_options,
649638
std::unique_ptr<CloudFileSystem>* fs);
650639

651-
const char* Name() const override { return "cloud-env"; }
652-
653-
const std::shared_ptr<FileSystem>& GetBaseFileSystem() const override {
654-
return base_fs_;
655-
}
656-
657-
Logger* GetLogger() const override { return info_log_.get(); }
658-
659-
const std::shared_ptr<CloudStorageProvider>& GetStorageProvider()
660-
const override {
661-
return cloud_fs_options.storage_provider;
662-
}
663-
664-
const std::shared_ptr<CloudLogController>& GetLogController() const {
665-
return cloud_fs_options.cloud_log_controller;
666-
}
667-
668-
const std::string& GetSrcBucketName() const override {
669-
return cloud_fs_options.src_bucket.GetBucketName();
670-
}
671-
const std::string& GetSrcObjectPath() const override {
672-
return cloud_fs_options.src_bucket.GetObjectPath();
673-
}
674-
bool HasSrcBucket() const override {
675-
return cloud_fs_options.src_bucket.IsValid();
676-
}
677-
678-
const std::string& GetDestBucketName() const override {
679-
return cloud_fs_options.dest_bucket.GetBucketName();
680-
}
681-
const std::string& GetDestObjectPath() const override {
682-
return cloud_fs_options.dest_bucket.GetObjectPath();
683-
}
684-
685-
bool HasDestBucket() const override {
686-
return cloud_fs_options.dest_bucket.IsValid();
687-
}
688-
bool SrcMatchesDest() const override {
689-
if (HasSrcBucket() && HasDestBucket()) {
690-
return cloud_fs_options.src_bucket == cloud_fs_options.dest_bucket;
691-
} else {
692-
return false;
693-
}
694-
}
695-
696-
// returns the options used to create this object
697-
const CloudFileSystemOptions& GetCloudFileSystemOptions() const override {
698-
return cloud_fs_options;
699-
}
700-
701640
// Create a new AWS file system.
702641
// src_bucket_name: bucket name suffix where db data is read from
703642
// src_object_prefix: all db objects in source bucket are prepended with this

‎include/rocksdb/cloud/cloud_file_system_impl.h

Lines changed: 53 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,12 @@ class CloudFileDeletionScheduler;
2222
//
2323
// The Cloud file system
2424
//
25-
class CloudFileSystemImpl : public CloudFileSystemEnv {
25+
class CloudFileSystemImpl : public CloudFileSystem {
2626
friend class CloudFileSystemEnv;
2727

2828
public:
29+
mutable std::shared_ptr<Logger> info_log_; // informational messages
30+
2931
static int RegisterAwsObjects(ObjectLibrary& library, const std::string& arg);
3032
// Constructor
3133
CloudFileSystemImpl(const CloudFileSystemOptions& options,
@@ -34,7 +36,12 @@ class CloudFileSystemImpl : public CloudFileSystemEnv {
3436

3537
virtual ~CloudFileSystemImpl();
3638
static const char* kClassName() { return kCloud(); }
37-
virtual const char* Name() const override { return kClassName(); }
39+
40+
const std::shared_ptr<FileSystem>& GetBaseFileSystem() const override {
41+
return base_fs_;
42+
}
43+
44+
const char* Name() const override { return kClassName(); }
3845

3946
IOStatus NewSequentialFile(const std::string& fname,
4047
const FileOptions& file_opts,
@@ -285,6 +292,9 @@ class CloudFileSystemImpl : public CloudFileSystemEnv {
285292
#endif
286293

287294
protected:
295+
CloudFileSystemOptions cloud_fs_options;
296+
std::shared_ptr<FileSystem> base_fs_; // The underlying file system
297+
288298
Status CheckValidity() const;
289299
// Status TEST_Initialize(const std::string& name) override;
290300
// The pathname that contains a list of all db's inside a bucket.
@@ -368,6 +378,47 @@ class CloudFileSystemImpl : public CloudFileSystemEnv {
368378
const std::string& dbname,
369379
const std::vector<std::string>& active_cookies) override;
370380

381+
public:
382+
// returns the options used to create this object
383+
const CloudFileSystemOptions& GetCloudFileSystemOptions() const override {
384+
return cloud_fs_options;
385+
}
386+
387+
const std::string& GetSrcBucketName() const override {
388+
return cloud_fs_options.src_bucket.GetBucketName();
389+
}
390+
const std::string& GetSrcObjectPath() const override {
391+
return cloud_fs_options.src_bucket.GetObjectPath();
392+
}
393+
bool HasSrcBucket() const override {
394+
return cloud_fs_options.src_bucket.IsValid();
395+
}
396+
397+
const std::string& GetDestBucketName() const override {
398+
return cloud_fs_options.dest_bucket.GetBucketName();
399+
}
400+
const std::string& GetDestObjectPath() const override {
401+
return cloud_fs_options.dest_bucket.GetObjectPath();
402+
}
403+
404+
bool HasDestBucket() const override {
405+
return cloud_fs_options.dest_bucket.IsValid();
406+
}
407+
bool SrcMatchesDest() const override {
408+
if (HasSrcBucket() && HasDestBucket()) {
409+
return cloud_fs_options.src_bucket == cloud_fs_options.dest_bucket;
410+
} else {
411+
return false;
412+
}
413+
}
414+
415+
const std::shared_ptr<CloudStorageProvider>& GetStorageProvider()
416+
const override {
417+
return cloud_fs_options.storage_provider;
418+
}
419+
420+
Logger* GetLogger() const override { return info_log_.get(); }
421+
371422
private:
372423
// Files are invisibile if:
373424
// - It's CLOUDMANFIEST file and cookie is not active. NOTE: empty cookie is

0 commit comments

Comments
 (0)