Skip to content
This repository was archived by the owner on Dec 21, 2023. It is now read-only.

Commit 847b07c

Browse files
author
Guihao Liang
authored
reduce duplicated code (#3103)
1 parent a6c7ce4 commit 847b07c

File tree

2 files changed

+46
-106
lines changed

2 files changed

+46
-106
lines changed

‎src/core/storage/fileio/s3_api.cpp‎

Lines changed: 42 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#include <boost/algorithm/string/predicate.hpp>
1616
#include <boost/algorithm/string/split.hpp>
1717
#include <boost/algorithm/string/trim.hpp>
18+
#include <boost/optional/optional_io.hpp>
1819
#include <boost/regex.hpp>
1920
#include <boost/tokenizer.hpp>
2021
#include <chrono>
@@ -122,6 +123,9 @@ bool bucket_name_valid(const std::string& bucket_name) {
122123

123124
} // anonymous namespace
124125

126+
/*
127+
* @param: parsed_url output parameter, its state will be modified
128+
*/
125129
S3Client init_aws_sdk_with_turi_env(s3url& parsed_url) {
126130
// s3 client config
127131
// DefaultCredentialProviderChain
@@ -426,38 +430,24 @@ std::string quote_and_escape_path(const std::string& path) {
426430
return ret;
427431
}
428432

429-
list_objects_response list_objects_impl(s3url parsed_url, std::string proxy,
433+
list_objects_response list_objects_impl(const s3url& parsed_url,
434+
std::string proxy,
430435
std::string endpoint) {
431-
// credentials
432-
Aws::Auth::AWSCredentials credentials(parsed_url.access_key_id.c_str(),
433-
parsed_url.secret_key.c_str());
436+
// do not modify the parsed_url because string_from_s3url() will
437+
// be called on it to retrieve its original url (prefix) in is_directory.
434438

435-
// s3 client config
436-
Aws::Client::ClientConfiguration clientConfiguration;
439+
// let init_aws_sdk to be aware of the endpoint to override
440+
auto temp_url = parsed_url;
437441

438-
if (turi::fileio::insecure_ssl_cert_checks()) {
439-
clientConfiguration.verifySSL = false;
442+
if (temp_url.endpoint.empty()) {
443+
temp_url.endpoint = endpoint;
440444
}
441445

442-
if (parsed_url.endpoint.empty()) {
443-
clientConfiguration.endpointOverride = endpoint.c_str();
444-
} else {
445-
clientConfiguration.endpointOverride = parsed_url.endpoint.c_str();
446+
if (!temp_url.sdk_proxy || temp_url.sdk_proxy->empty()) {
447+
temp_url.sdk_proxy = proxy;
446448
}
447449

448-
clientConfiguration.proxyHost = proxy.c_str();
449-
clientConfiguration.requestTimeoutMs = 5 * 60000;
450-
clientConfiguration.connectTimeoutMs = 20000;
451-
std::string region = fileio::get_region_name_from_endpoint(
452-
clientConfiguration.endpointOverride.c_str());
453-
if (!region.empty()) {
454-
clientConfiguration.region = region.c_str();
455-
parsed_url.sdk_region = region;
456-
}
457-
458-
S3Client client(credentials, clientConfiguration,
459-
Aws::Client::AWSAuthV4Signer::PayloadSigningPolicy::Never,
460-
false);
450+
S3Client client = init_aws_sdk_with_turi_env(temp_url);
461451

462452
list_objects_response ret;
463453

@@ -522,8 +512,7 @@ list_objects_response list_objects_impl(s3url parsed_url, std::string proxy,
522512
if (n_retry == 3) {
523513
// amend the error msg on the last retry failure
524514
std::stringstream ss;
525-
reportS3ErrorDetailed(ss, parsed_url, S3Operation::List,
526-
clientConfiguration, outcome)
515+
reportS3ErrorDetailed(ss, temp_url, S3Operation::List, outcome)
527516
<< std::endl;
528517
ret.error = ss.str();
529518
logstream(LOG_DEBUG)
@@ -536,8 +525,7 @@ list_objects_response list_objects_impl(s3url parsed_url, std::string proxy,
536525

537526
} else {
538527
std::stringstream ss;
539-
reportS3ErrorDetailed(ss, parsed_url, S3Operation::List,
540-
clientConfiguration, outcome)
528+
reportS3ErrorDetailed(ss, temp_url, S3Operation::List, outcome)
541529
<< std::endl;
542530
ret.error = ss.str();
543531
logstream(LOG_DEBUG)
@@ -566,47 +554,25 @@ list_objects_response list_objects_impl(s3url parsed_url, std::string proxy,
566554
}
567555

568556
/// returns an error string on failure. Empty string on success
569-
std::string delete_object_impl(s3url parsed_url, std::string proxy,
557+
std::string delete_object_impl(const s3url& parsed_url, std::string proxy,
570558
std::string endpoint) {
571-
std::string ret;
572-
573-
// credentials
574-
Aws::Auth::AWSCredentials credentials(parsed_url.access_key_id.c_str(),
575-
parsed_url.secret_key.c_str());
576-
577-
// s3 client config
578-
Aws::Client::ClientConfiguration clientConfiguration;
579-
580-
clientConfiguration.requestTimeoutMs = 5 * 60000;
581-
clientConfiguration.connectTimeoutMs = 20000;
559+
// do not modify the parsed_url because string_from_s3url() will
560+
// be called on it to retrieve its original url (prefix) in is_directory.
582561

583-
if (turi::fileio::insecure_ssl_cert_checks()) {
584-
clientConfiguration.verifySSL = false;
585-
}
562+
// let init_aws_sdk to be aware of the endpoint to override
563+
auto temp_url = parsed_url;
586564

587-
if (parsed_url.endpoint.empty()) {
588-
clientConfiguration.endpointOverride = endpoint.c_str();
589-
parsed_url.sdk_endpoint = endpoint;
590-
} else {
591-
clientConfiguration.endpointOverride = parsed_url.endpoint.c_str();
565+
if (temp_url.endpoint.empty()) {
566+
temp_url.endpoint = endpoint;
592567
}
593568

594-
if (!proxy.empty()) {
595-
clientConfiguration.proxyHost = proxy.c_str();
596-
parsed_url.sdk_proxy = proxy;
569+
if (!temp_url.sdk_proxy || temp_url.sdk_proxy->empty()) {
570+
temp_url.sdk_proxy = proxy;
597571
}
598572

599-
std::string region = fileio::get_region_name_from_endpoint(
600-
clientConfiguration.endpointOverride.c_str());
601-
if (!region.empty()) {
602-
clientConfiguration.region = region.c_str();
603-
parsed_url.sdk_region = region;
604-
}
573+
S3Client client = init_aws_sdk_with_turi_env(temp_url);
605574

606-
S3Client client(credentials, clientConfiguration,
607-
/* default value */
608-
Aws::Client::AWSAuthV4Signer::PayloadSigningPolicy::Never,
609-
/* use virtual address */ false);
575+
std::string ret;
610576

611577
Aws::S3::Model::DeleteObjectRequest request;
612578
request.WithBucket(parsed_url.bucket.c_str());
@@ -615,8 +581,7 @@ std::string delete_object_impl(s3url parsed_url, std::string proxy,
615581
auto outcome = client.DeleteObject(request);
616582
if (!outcome.IsSuccess()) {
617583
std::stringstream ss;
618-
reportS3ErrorDetailed(ss, parsed_url, S3Operation::Delete,
619-
clientConfiguration, outcome)
584+
reportS3ErrorDetailed(ss, temp_url, S3Operation::Delete, outcome)
620585
<< std::endl;
621586
ret = ss.str();
622587
}
@@ -625,46 +590,25 @@ std::string delete_object_impl(s3url parsed_url, std::string proxy,
625590
}
626591

627592
/// returns an error string on failure. Empty string on success
628-
std::string delete_prefix_impl(s3url parsed_url, std::string proxy,
593+
std::string delete_prefix_impl(const s3url& parsed_url, std::string proxy,
629594
std::string endpoint) {
630-
// List objects and then create a DeleteObjects request from the resulting
631-
std::string ret;
595+
// do not modify the parsed_url because string_from_s3url() will
596+
// be called on it to retrieve its original url (prefix) in is_directory.
632597

633-
// credentials
634-
Aws::Auth::AWSCredentials credentials(parsed_url.access_key_id.c_str(),
635-
parsed_url.secret_key.c_str());
598+
// let init_aws_sdk to be aware of the endpoint to override
599+
auto temp_url = parsed_url;
636600

637-
// s3 client config
638-
Aws::Client::ClientConfiguration clientConfiguration;
639-
if (turi::fileio::insecure_ssl_cert_checks()) {
640-
clientConfiguration.verifySSL = false;
601+
if (temp_url.endpoint.empty()) {
602+
temp_url.endpoint = endpoint;
641603
}
642604

643-
if (parsed_url.endpoint.empty()) {
644-
clientConfiguration.endpointOverride = endpoint.c_str();
645-
// for report
646-
parsed_url.endpoint = endpoint;
647-
} else {
648-
clientConfiguration.endpointOverride = parsed_url.endpoint.c_str();
605+
if (!temp_url.sdk_proxy || temp_url.sdk_proxy->empty()) {
606+
temp_url.sdk_proxy = proxy;
649607
}
650608

651-
if (proxy.size()) {
652-
clientConfiguration.proxyHost = proxy.c_str();
653-
parsed_url.sdk_proxy = proxy;
654-
}
655-
clientConfiguration.requestTimeoutMs = 5 * 60000;
656-
clientConfiguration.connectTimeoutMs = 20000;
657-
std::string region = fileio::get_region_name_from_endpoint(
658-
clientConfiguration.endpointOverride.c_str());
659-
if (!region.empty()) {
660-
clientConfiguration.region = region.c_str();
661-
parsed_url.sdk_region = region;
662-
}
609+
S3Client client = init_aws_sdk_with_turi_env(temp_url);
663610

664-
S3Client client(credentials, clientConfiguration,
665-
/* default value */
666-
Aws::Client::AWSAuthV4Signer::PayloadSigningPolicy::Never,
667-
/* use virtual address */ false);
611+
std::string ret;
668612

669613
Aws::S3::Model::ListObjectsV2Request request;
670614
request.WithBucket(parsed_url.bucket.c_str());
@@ -695,8 +639,7 @@ std::string delete_prefix_impl(s3url parsed_url, std::string proxy,
695639

696640
} else {
697641
std::stringstream ss;
698-
reportS3ErrorDetailed(ss, parsed_url, S3Operation::List,
699-
clientConfiguration, outcome)
642+
reportS3ErrorDetailed(ss, temp_url, S3Operation::List, outcome)
700643
<< std::endl;
701644
ret = ss.str();
702645
}
@@ -711,8 +654,7 @@ std::string delete_prefix_impl(s3url parsed_url, std::string proxy,
711654
auto outcome = client.DeleteObjects(delRequest);
712655
if (!outcome.IsSuccess()) {
713656
std::stringstream ss;
714-
reportS3ErrorDetailed(ss, parsed_url, S3Operation::Delete,
715-
clientConfiguration, outcome)
657+
reportS3ErrorDetailed(ss, parsed_url, S3Operation::Delete, outcome)
716658
<< std::endl;
717659
ret = ss.str();
718660
}

‎src/core/storage/fileio/s3_api.hpp‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,6 @@ struct s3url {
7373
// this is embeded form
7474
// something like: s3://s3.amazonaws.com/bucket/object/name
7575
if (!endpoint.empty()) {
76-
ret.append(1, ':');
7776
ret.append(endpoint);
7877
ret.append(1, '/');
7978
}
@@ -284,11 +283,10 @@ struct S3Operation {
284283
template <class Response>
285284
std::ostream& reportS3Error(std::ostream& ss, const s3url& parsed_url,
286285
S3Operation::ops_enum operation,
287-
const Aws::Client::ClientConfiguration& config,
288286
const Response& outcome) {
289287
auto error = outcome.GetError();
290-
ss << "('" << parsed_url << ", proxy: '" << config.proxyHost << "', region: '"
291-
<< config.region << "')"
288+
ss << "('" << parsed_url << ", proxy: '" << parsed_url.sdk_proxy
289+
<< "', region: '" << parsed_url.sdk_region << "')"
292290
<< " Error while performing " << S3Operation::toString(operation)
293291
<< ". Error Name: " << error.GetExceptionName()
294292
<< ". Error Message: " << error.GetMessage()
@@ -297,8 +295,8 @@ std::ostream& reportS3Error(std::ostream& ss, const s3url& parsed_url,
297295
return ss;
298296
}
299297

300-
#define reportS3ErrorDetailed(ss, parsed_url, operation, config, outcome) \
301-
reportS3Error(ss, parsed_url, operation, config, outcome) \
298+
#define reportS3ErrorDetailed(ss, parsed_url, operation, outcome) \
299+
reportS3Error(ss, parsed_url, operation, outcome) \
302300
<< " in " << __FILE__ << " at " << __LINE__
303301

304302
} // namespace turi

0 commit comments

Comments
 (0)