Skip to content

Commit 8785588

Browse files
author
Duc Hieu Pham
committed
Revert "Fix a bug in checking CopyObject response"
Summary: This reverts commit f314650. Test Plan: N/A. just revert Reviewers: igor, dhruba, #platform Reviewed By: dhruba, #platform Differential Revision: https://rockset.phacility.com/D6336
1 parent f314650 commit 8785588

File tree

2 files changed

+11
-60
lines changed

2 files changed

+11
-60
lines changed

cloud/aws/aws_s3.cc

Lines changed: 9 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,6 @@
5252
#include "util/string_util.h"
5353

5454
namespace rocksdb {
55-
5655
#ifdef USE_AWS
5756
class CloudRequestCallbackGuard {
5857
public:
@@ -160,25 +159,12 @@ class AwsS3ClientWrapper {
160159
return outcome;
161160
}
162161

163-
// The copy RPC is only successful iff outcome is success and etags between
164-
// src and dst match.
165-
static bool isCopyCloudObjectSuccess(
166-
Aws::S3::Model::CopyObjectOutcome& outcome, const Aws::String& src_etag) {
167-
bool success = false;
168-
if (outcome.IsSuccess()) {
169-
const auto& detail = outcome.GetResult().GetCopyObjectResultDetails();
170-
success = (detail.ETagHasBeenSet() && detail.GetETag() == src_etag);
171-
}
172-
return success;
173-
}
174-
175162
Aws::S3::Model::CopyObjectOutcome CopyCloudObject(
176-
const Aws::S3::Model::CopyObjectRequest& request,
177-
const Aws::String& src_etag) {
163+
const Aws::S3::Model::CopyObjectRequest& request) {
178164
CloudRequestCallbackGuard t(cloud_request_callback_.get(),
179165
CloudRequestOpType::kCopyOp);
180166
auto outcome = client_->CopyObject(request);
181-
t.SetSuccess(isCopyCloudObjectSuccess(outcome, src_etag));
167+
t.SetSuccess(outcome.IsSuccess());
182168
return outcome;
183169
}
184170

@@ -760,25 +746,6 @@ Status S3StorageProvider::CopyCloudObject(const std::string& bucket_name_src,
760746

761747
Aws::String src_url = src_bucket + src_object;
762748

763-
// Get the metadata of the source object. We need the metadata so that we can
764-
// compare the ETag between the source object and dest object. Only when these
765-
// ETags match can we conclude this CopyObject operations succeed.
766-
//
767-
// More details:
768-
// https://sdk.amazonaws.com/cpp/api/LATEST/class_aws_1_1_s3_1_1_model_1_1_copy_object_result_details.html
769-
// https://docs.aws.amazon.com/AmazonS3/latest/API/API_CopyObject.html
770-
Aws::S3::Model::HeadObjectRequest head_request;
771-
head_request.SetBucket(ToAwsString(bucket_name_src));
772-
head_request.SetKey(ToAwsString(object_path_src));
773-
774-
auto head_outcome = s3client_->HeadObject(head_request);
775-
if (!head_outcome.IsSuccess()) {
776-
return Status::NotFound(
777-
"[s3] CopyCloudObject: Fail to get metadata of src object: %s",
778-
head_outcome.GetError().GetMessage().c_str());
779-
}
780-
const auto& src_etag = head_outcome.GetResult().GetETag();
781-
782749
// create copy request
783750
Aws::S3::Model::CopyObjectRequest request;
784751
request.SetCopySource(src_url);
@@ -788,29 +755,15 @@ Status S3StorageProvider::CopyCloudObject(const std::string& bucket_name_src,
788755

789756
// execute request
790757
Aws::S3::Model::CopyObjectOutcome outcome =
791-
s3client_->CopyCloudObject(request, src_etag);
792-
bool isSuccess =
793-
AwsS3ClientWrapper::isCopyCloudObjectSuccess(outcome, src_etag);
758+
s3client_->CopyCloudObject(request);
759+
bool isSuccess = outcome.IsSuccess();
794760
if (!isSuccess) {
795-
// S3 CopyObject RPC can report errors in 2 ways:
796-
// 1. outcome.IsSuccess() = false
797-
// 2. outcome.IsSuccess() = true but the outcome's ETag doesn't match the
798-
// source's ETag.
799-
if (!outcome.IsSuccess()) {
800-
const Aws::Client::AWSError<Aws::S3::S3Errors>& error =
801-
outcome.GetError();
802-
std::string errmsg(error.GetMessage().c_str());
803-
Log(InfoLogLevel::ERROR_LEVEL, env_->info_log_,
804-
"[s3] S3WritableFile src path %s error in copying to %s %s",
805-
src_url.c_str(), dest_object.c_str(), errmsg.c_str());
806-
return Status::IOError(dest_object.c_str(), errmsg.c_str());
807-
}
808-
761+
const Aws::Client::AWSError<Aws::S3::S3Errors>& error = outcome.GetError();
762+
std::string errmsg(error.GetMessage().c_str());
809763
Log(InfoLogLevel::ERROR_LEVEL, env_->info_log_,
810-
"[s3] S3WritableFile src path %s error in copying to %s: CopyObject "
811-
"etags don't match",
812-
src_url.c_str(), dest_object.c_str());
813-
return Status::IOError("CopyObject S3 etags don't match");
764+
"[s3] S3WritableFile src path %s error in copying to %s %s",
765+
src_url.c_str(), dest_object.c_str(), errmsg.c_str());
766+
return Status::IOError(dest_object.c_str(), errmsg.c_str());
814767
}
815768
Log(InfoLogLevel::INFO_LEVEL, env_->info_log_,
816769
"[s3] S3WritableFile src path %s copied to %s %s", src_url.c_str(),

cloud/db_cloud_test.cc

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
// Copyright (c) 2017 Rockset
22

3-
#include <gtest/gtest.h>
4-
53
#ifndef ROCKSDB_LITE
64

75
#ifdef USE_AWS
86

7+
#include "rocksdb/cloud/db_cloud.h"
8+
99
#include <algorithm>
1010
#include <chrono>
1111
#include <cinttypes>
@@ -18,8 +18,6 @@
1818
#include "file/filename.h"
1919
#include "logging/logging.h"
2020
#include "rocksdb/cloud/cloud_storage_provider.h"
21-
#include "rocksdb/cloud/db_cloud.h"
22-
#include "rocksdb/env.h"
2321
#include "rocksdb/options.h"
2422
#include "rocksdb/status.h"
2523
#include "rocksdb/table.h"

0 commit comments

Comments
 (0)