Skip to content

Commit 516c3e0

Browse files
mrambachermrambacher
committed
Add HasXXXBucket Methods
Replaces checks that were not consistent throughout the codebase.
1 parent 915dc86 commit 516c3e0

File tree

5 files changed

+86
-87
lines changed

5 files changed

+86
-87
lines changed

‎cloud/aws/aws_env.cc‎

Lines changed: 28 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -264,10 +264,7 @@ AwsEnv::AwsEnv(Env* underlying_env,
264264
: CloudEnvImpl(_cloud_env_options,
265265
underlying_env),
266266
info_log_(info_log),
267-
running_(true),
268-
has_src_bucket_(false),
269-
has_dest_bucket_(false),
270-
dest_equal_src_(false) {
267+
running_(true) {
271268
Aws::InitAPI(Aws::SDKOptions());
272269
if (cloud_env_options.src_bucket.GetRegion().empty() ||
273270
cloud_env_options.dest_bucket.GetRegion().empty()) {
@@ -282,14 +279,6 @@ AwsEnv::AwsEnv(Env* underlying_env,
282279
cloud_env_options.dest_bucket.SetRegion(region);
283280
}
284281
}
285-
has_src_bucket_ = cloud_env_options.src_bucket.IsValid();
286-
has_dest_bucket_ = cloud_env_options.dest_bucket.IsValid();
287-
288-
// Do we have two unique buckets?
289-
dest_equal_src_ = has_src_bucket_ && has_dest_bucket_ &&
290-
GetSrcBucketName() == GetDestBucketName() &&
291-
GetSrcObjectPath() == GetDestObjectPath();
292-
293282

294283
unique_ptr<Aws::Auth::AWSCredentials> creds;
295284
if (!cloud_env_options.credentials.access_key_id.empty() &&
@@ -328,7 +317,7 @@ AwsEnv::AwsEnv(Env* underlying_env,
328317
}
329318

330319
// TODO: support buckets being in different regions
331-
if (!dest_equal_src_ && has_src_bucket_ && has_dest_bucket_) {
320+
if (!SrcMatchesDest() && HasSrcBucket() && HasDestBucket()) {
332321
if (cloud_env_options.src_bucket.GetRegion() == cloud_env_options.dest_bucket.GetRegion()) {
333322
// alls good
334323
} else {
@@ -373,7 +362,7 @@ AwsEnv::AwsEnv(Env* underlying_env,
373362
}
374363

375364
// create dest bucket if specified
376-
if (has_dest_bucket_) {
365+
if (HasDestBucket()) {
377366
if (S3WritableFile::BucketExistsInS3(s3client_, GetDestBucketName(),
378367
bucket_location_)
379368
.ok()) {
@@ -555,10 +544,10 @@ Status AwsEnv::NewSequentialFile(const std::string& logical_fname,
555544
if (!st.ok()) {
556545
if (cloud_env_options.keep_local_sst_files || !sstfile) {
557546
// copy the file to the local storage if keep_local_sst_files is true
558-
if (has_dest_bucket_) {
547+
if (HasDestBucket()) {
559548
st = GetObject(GetDestBucketName(), destname(fname), fname);
560549
}
561-
if (!st.ok() && has_src_bucket_ && !dest_equal_src_) {
550+
if (!st.ok() && HasSrcBucket() && !SrcMatchesDest()) {
562551
st = GetObject(GetSrcBucketName(), srcname(fname), fname);
563552
}
564553
if (st.ok()) {
@@ -567,10 +556,10 @@ Status AwsEnv::NewSequentialFile(const std::string& logical_fname,
567556
}
568557
} else {
569558
unique_ptr<S3ReadableFile> file;
570-
if (!st.ok() && has_dest_bucket_) { // read from destination S3
559+
if (!st.ok() && HasDestBucket()) { // read from destination S3
571560
st = NewS3ReadableFile(GetDestBucketName(), destname(fname), &file);
572561
}
573-
if (!st.ok() && has_src_bucket_) { // read from src bucket
562+
if (!st.ok() && HasSrcBucket()) { // read from src bucket
574563
st = NewS3ReadableFile(GetSrcBucketName(), srcname(fname), &file);
575564
}
576565
if (st.ok()) {
@@ -636,10 +625,10 @@ Status AwsEnv::NewRandomAccessFile(const std::string& logical_fname,
636625
if (cloud_env_options.keep_local_sst_files || !sstfile) {
637626
if (!st.ok()) {
638627
// copy the file to the local storage if keep_local_sst_files is true
639-
if (has_dest_bucket_) {
628+
if (HasDestBucket()) {
640629
st = GetObject(GetDestBucketName(), destname(fname), fname);
641630
}
642-
if (!st.ok() && has_src_bucket_ && !dest_equal_src_) {
631+
if (!st.ok() && HasSrcBucket() && !SrcMatchesDest()) {
643632
st = GetObject(GetSrcBucketName(), srcname(fname), fname);
644633
}
645634
if (st.ok()) {
@@ -657,15 +646,15 @@ Status AwsEnv::NewRandomAccessFile(const std::string& logical_fname,
657646
return stax;
658647
}
659648
stax = Status::NotFound();
660-
if (has_dest_bucket_) {
649+
if (HasDestBucket()) {
661650
stax = HeadObject(GetDestBucketName(), destname(fname), nullptr,
662651
&remote_size, nullptr);
663652
}
664-
if (stax.IsNotFound() && has_src_bucket_) {
653+
if (stax.IsNotFound() && HasSrcBucket()) {
665654
stax = HeadObject(GetSrcBucketName(), srcname(fname), nullptr,
666655
&remote_size, nullptr);
667656
}
668-
if (stax.IsNotFound() && !has_dest_bucket_) {
657+
if (stax.IsNotFound() && !HasDestBucket()) {
669658
// It is legal for file to not be present in S3 if destination bucket
670659
// is not set.
671660
} else if (!stax.ok() || remote_size != local_size) {
@@ -681,10 +670,10 @@ Status AwsEnv::NewRandomAccessFile(const std::string& logical_fname,
681670
// true, we will never use S3ReadableFile to read; we copy the file
682671
// locally and read using base_env.
683672
unique_ptr<S3ReadableFile> file;
684-
if (!st.ok() && has_dest_bucket_) {
673+
if (!st.ok() && HasDestBucket()) {
685674
st = NewS3ReadableFile(GetDestBucketName(), destname(fname), &file);
686675
}
687-
if (!st.ok() && has_src_bucket_) {
676+
if (!st.ok() && HasSrcBucket()) {
688677
st = NewS3ReadableFile(GetSrcBucketName(), srcname(fname), &file);
689678
}
690679
if (st.ok()) {
@@ -734,7 +723,7 @@ Status AwsEnv::NewWritableFile(const std::string& logical_fname,
734723

735724
Status s;
736725

737-
if (has_dest_bucket_ && (sstfile || identity || manifest)) {
726+
if (HasDestBucket() && (sstfile || identity || manifest)) {
738727
unique_ptr<S3WritableFile> f(
739728
new S3WritableFile(this, fname, GetDestBucketName(), destname(fname),
740729
options, cloud_env_options));
@@ -834,10 +823,10 @@ Status AwsEnv::FileExists(const std::string& logical_fname) {
834823
if (sstfile || manifest || identity) {
835824
// We read first from local storage and then from cloud storage.
836825
st = base_env_->FileExists(fname);
837-
if (st.IsNotFound() && has_dest_bucket_) {
826+
if (st.IsNotFound() && HasDestBucket()) {
838827
st = ExistsObject(GetDestBucketName(), destname(fname));
839828
}
840-
if (!st.ok() && has_src_bucket_) {
829+
if (!st.ok() && HasSrcBucket()) {
841830
st = ExistsObject(GetSrcBucketName(), srcname(fname));
842831
}
843832
} else if (logfile && !cloud_env_options.keep_local_log_files) {
@@ -1026,7 +1015,7 @@ Status AwsEnv::GetChildren(const std::string& path,
10261015

10271016
// Fetch the list of children from both buckets in S3
10281017
Status st;
1029-
if (has_src_bucket_) {
1018+
if (HasSrcBucket()) {
10301019
st = GetChildrenFromS3(GetSrcObjectPath(), GetSrcBucketName(), result);
10311020
if (!st.ok()) {
10321021
Log(InfoLogLevel::ERROR_LEVEL, info_log_,
@@ -1035,7 +1024,7 @@ Status AwsEnv::GetChildren(const std::string& path,
10351024
return st;
10361025
}
10371026
}
1038-
if (has_dest_bucket_ && !dest_equal_src_) {
1027+
if (HasDestBucket() && !SrcMatchesDest()) {
10391028
st = GetChildrenFromS3(GetDestObjectPath(), GetDestBucketName(), result);
10401029
if (!st.ok()) {
10411030
Log(InfoLogLevel::ERROR_LEVEL, info_log_,
@@ -1135,7 +1124,7 @@ Status AwsEnv::DeleteFile(const std::string& logical_fname) {
11351124
Status st;
11361125
// Delete from destination bucket and local dir
11371126
if (sstfile || manifest || identity) {
1138-
if (has_dest_bucket_) {
1127+
if (HasDestBucket()) {
11391128
// add the remote file deletion to the queue
11401129
st = DeleteCloudFileFromDest(basename(fname));
11411130
}
@@ -1164,7 +1153,7 @@ Status AwsEnv::DeleteFile(const std::string& logical_fname) {
11641153
}
11651154

11661155
Status AwsEnv::DeleteCloudFileFromDest(const std::string& fname) {
1167-
assert(!GetDestBucketName().empty());
1156+
assert(HasDestBucket());
11681157
auto base = basename(fname);
11691158
// add the job to delete the file in 1 hour
11701159
auto doDeleteFile = [this, base]() {
@@ -1302,11 +1291,11 @@ Status AwsEnv::GetFileSize(const std::string& logical_fname, uint64_t* size) {
13021291
} else {
13031292
st = Status::NotFound();
13041293
// Get file length from S3
1305-
if (has_dest_bucket_) {
1294+
if (HasDestBucket()) {
13061295
st = HeadObject(GetDestBucketName(), destname(fname), nullptr, size,
13071296
nullptr);
13081297
}
1309-
if (st.IsNotFound() && has_src_bucket_) {
1298+
if (st.IsNotFound() && HasSrcBucket()) {
13101299
st = HeadObject(GetSrcBucketName(), srcname(fname), nullptr, size,
13111300
nullptr);
13121301
}
@@ -1349,11 +1338,11 @@ Status AwsEnv::GetFileModificationTime(const std::string& logical_fname,
13491338
st = base_env_->GetFileModificationTime(fname, time);
13501339
} else {
13511340
st = Status::NotFound();
1352-
if (has_dest_bucket_) {
1341+
if (HasDestBucket()) {
13531342
st = HeadObject(GetDestBucketName(), destname(fname), nullptr,
13541343
nullptr, time);
13551344
}
1356-
if (st.IsNotFound() && has_src_bucket_) {
1345+
if (st.IsNotFound() && HasSrcBucket()) {
13571346
st = HeadObject(GetSrcBucketName(), srcname(fname), nullptr, nullptr,
13581347
time);
13591348
}
@@ -1419,12 +1408,12 @@ Status AwsEnv::RenameFile(const std::string& logical_src,
14191408
assert(0);
14201409
return Status::NotSupported(Slice(src), Slice(target));
14211410

1422-
} else if (!identity || !has_dest_bucket_) {
1411+
} else if (!identity || !HasDestBucket()) {
14231412
return base_env_->RenameFile(src, target);
14241413
}
14251414
// Only ID file should come here
14261415
assert(identity);
1427-
assert(has_dest_bucket_);
1416+
assert(HasDestBucket());
14281417
assert(basename(target) == "IDENTITY");
14291418

14301419
// Save Identity to S3
@@ -1442,7 +1431,7 @@ Status AwsEnv::RenameFile(const std::string& logical_src,
14421431

14431432
Status AwsEnv::LinkFile(const std::string& src, const std::string& target) {
14441433
// We only know how to link file if both src and dest buckets are empty
1445-
if (has_dest_bucket_ || has_src_bucket_) {
1434+
if (HasDestBucket() || HasSrcBucket()) {
14461435
return Status::NotSupported();
14471436
}
14481437
auto src_remapped = RemapFilename(src);

‎cloud/aws/aws_env.h‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -239,8 +239,6 @@ class AwsEnv : public CloudEnvImpl {
239239

240240
bool IsRunning() const { return running_; }
241241

242-
243-
244242
std::string GetWALCacheDir();
245243

246244
std::shared_ptr<Logger> info_log_; // informational messages
@@ -352,15 +350,6 @@ class AwsEnv : public CloudEnvImpl {
352350

353351
Aws::S3::Model::BucketLocationConstraint bucket_location_;
354352

355-
// Is there a src bucket specified?
356-
bool has_src_bucket_;
357-
358-
// Is there a dest bucket specified?
359-
bool has_dest_bucket_;
360-
361-
// Is the src bucket different from the destination bucket?
362-
bool dest_equal_src_;
363-
364353
Status status();
365354

366355
// Delete the specified path from S3

‎cloud/cloud_env.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ std::string CloudEnvImpl::RemapFilename(const std::string& logical_path) const {
166166

167167
Status CloudEnvImpl::DeleteInvisibleFiles(const std::string& dbname) {
168168
Status s;
169-
if (!GetDestBucketName().empty()) {
169+
if (HasDestBucket()) {
170170
BucketObjectMetadata metadata;
171171
s = ListObjects(GetDestBucketName(), GetDestObjectPath(), &metadata);
172172
if (!s.ok()) {

0 commit comments

Comments
 (0)