From 11ccadb95b91e2f830260841b5bff552484cf205 Mon Sep 17 00:00:00 2001 From: Alexandre Baiao Date: Fri, 20 Dec 2019 10:56:43 -0300 Subject: [PATCH 1/7] Fixes in retry --- include/blob/blob_client.h | 9 +++++++++ include/retry.h | 31 +++++++++++++++++++++---------- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/include/blob/blob_client.h b/include/blob/blob_client.h index d53645f..0390ded 100644 --- a/include/blob/blob_client.h +++ b/include/blob/blob_client.h @@ -53,6 +53,15 @@ namespace azure { namespace storage_lite { m_client = std::make_shared(max_concurrency, ca_path); } + blob_client(std::shared_ptr account, + std::shared_ptr context, + std::shared_ptr client) + : m_account(account), + m_context(context), + m_client(client) + { + } + /// /// Gets the curl client used to execute requests. /// diff --git a/include/retry.h b/include/retry.h index 3831887..acba6f8 100644 --- a/include/retry.h +++ b/include/retry.h @@ -78,20 +78,22 @@ namespace azure { namespace storage_lite { class retry_policy final : public retry_policy_base { public: + + retry_policy(const int maximum_retries = 3, const std::chrono::seconds base_timeout = std::chrono::seconds{10}) + : m_maximum_retries(maximum_retries), + m_base_timeout(base_timeout) + { + } + retry_info evaluate(const retry_context &context) const override { - if (context.numbers() == 0) - { - return retry_info(true, std::chrono::seconds(0)); - } - else if (context.numbers() < 26 && can_retry(context.result())) + if (context.numbers() >= m_maximum_retries || + !can_retry(context.result())) { - double delay = (pow(1.2, context.numbers()-1)-1); - delay = std::min(delay, 60.0); // Maximum backoff delay of 1 minute - delay *= (((double)rand())/RAND_MAX)/2 + 0.75; - return retry_info(true, std::chrono::seconds((int)delay)); + return retry_info(false, std::chrono::seconds(0)); } - return retry_info(false, std::chrono::seconds(0)); + + return retry_info(true, calculate_new_delay(context)); } private: @@ -99,6 +101,15 @@ namespace azure { namespace storage_lite { { return retryable(code); } + + std::chrono::seconds calculate_new_delay(const retry_context &context) + { + // (1 << N) == 2^N + return ((context.numbers() == 0) ? 0 : (m_base_timeout * (1 << context.numbers()))); + } + + const int m_maximum_retries; + const std::chrono::seconds m_base_timeout; }; }} // azure::storage_lite From 3ce274e678dfda3c0a88660f0c8cf216e1015dcc Mon Sep 17 00:00:00 2001 From: Alexandre Baiao Date: Fri, 20 Dec 2019 13:50:38 -0300 Subject: [PATCH 2/7] Some refactoring --- include/blob/blob_client.h | 6 +++--- include/retry.h | 5 +++-- src/blob/blob_client.cpp | 18 ++++++++++-------- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/include/blob/blob_client.h b/include/blob/blob_client.h index 0390ded..4f0f606 100644 --- a/include/blob/blob_client.h +++ b/include/blob/blob_client.h @@ -56,9 +56,9 @@ namespace azure { namespace storage_lite { blob_client(std::shared_ptr account, std::shared_ptr context, std::shared_ptr client) - : m_account(account), - m_context(context), - m_client(client) + : m_client(client), + m_account(account), + m_context(context) { } diff --git a/include/retry.h b/include/retry.h index acba6f8..6d7ac83 100644 --- a/include/retry.h +++ b/include/retry.h @@ -102,10 +102,11 @@ namespace azure { namespace storage_lite { return retryable(code); } - std::chrono::seconds calculate_new_delay(const retry_context &context) + std::chrono::seconds calculate_new_delay(const retry_context &context) const { // (1 << N) == 2^N - return ((context.numbers() == 0) ? 0 : (m_base_timeout * (1 << context.numbers()))); + return ((context.numbers() == 0) ? std::chrono::seconds(0) + : (m_base_timeout * (1 << context.numbers()))); } const int m_maximum_retries; diff --git a/src/blob/blob_client.cpp b/src/blob/blob_client.cpp index 046a88d..b00ab6c 100644 --- a/src/blob/blob_client.cpp +++ b/src/blob/blob_client.cpp @@ -196,12 +196,13 @@ std::future> blob_client::get_container_prop container_property properties(true); properties.etag = http->get_response_header(constants::header_etag); - auto& headers = http->get_response_headers(); - for (auto iter = headers.begin(); iter != headers.end(); ++iter) + const auto& headers = http->get_response_headers(); + properties.metadata.reserve(headers.size()); + for (const auto& header : headers) { - if (iter->first.find(constants::header_ms_meta_prefix) == 0) + if (header.first.find(constants::header_ms_meta_prefix) == 0) { - properties.metadata.push_back(std::make_pair(iter->first.substr(constants::header_ms_meta_prefix_size), iter->second)); + properties.metadata.emplace_back(header.first.substr(constants::header_ms_meta_prefix_size), header.second); } } return storage_outcome(properties); @@ -283,13 +284,14 @@ std::future> blob_client::get_blob_properties(con properties.size = std::stoull(contentLength, &sz, 0); } - auto& headers = http->get_response_headers(); - for (auto iter = headers.begin(); iter != headers.end(); ++iter) + const auto& headers = http->get_response_headers(); + properties.metadata.reserve(headers.size()); + for (const auto& header : headers) { - if (iter->first.find(constants::header_ms_meta_prefix) == 0) + if (header.first.find(constants::header_ms_meta_prefix) == 0) { // We need to strip ten characters from the front of the key to account for "x-ms-meta-". - properties.metadata.push_back(std::make_pair(iter->first.substr(constants::header_ms_meta_prefix_size), iter->second)); + properties.metadata.emplace_back(header.first.substr(constants::header_ms_meta_prefix_size), header.second); } } return storage_outcome(properties); From f80fd9e1548debe1734c2a72f02ed6af8b7b82a3 Mon Sep 17 00:00:00 2001 From: Alexandre Baiao Date: Fri, 20 Dec 2019 15:39:10 -0300 Subject: [PATCH 3/7] Const correctness and modernization --- include/get_container_property_request_base.h | 2 +- src/blob/blob_client.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/include/get_container_property_request_base.h b/include/get_container_property_request_base.h index bd12c4e..a0adbe3 100644 --- a/include/get_container_property_request_base.h +++ b/include/get_container_property_request_base.h @@ -33,7 +33,7 @@ namespace azure { namespace storage_lite { m_valid = valid; } - bool valid() + bool valid() const { return m_valid; } diff --git a/src/blob/blob_client.cpp b/src/blob/blob_client.cpp index b00ab6c..11542af 100644 --- a/src/blob/blob_client.cpp +++ b/src/blob/blob_client.cpp @@ -75,7 +75,7 @@ storage_outcome blob_client::get_chunk_to_stream_sync(const std: property.etag = http->get_response_header(constants::header_etag); property.totalSize = get_length_from_content_range(http->get_response_header(constants::header_content_range)); std::istringstream(http->get_response_header(constants::header_content_length)) >> property.size; - property.last_modified = curl_getdate(http->get_response_header(constants::header_last_modified).c_str(), NULL); + property.last_modified = curl_getdate(http->get_response_header(constants::header_last_modified).c_str(), nullptr); return storage_outcome(property); } return storage_outcome(storage_error(response.error())); @@ -276,7 +276,7 @@ std::future> blob_client::get_blob_properties(con properties.content_type = http->get_response_header(constants::header_content_type); properties.etag = http->get_response_header(constants::header_etag); properties.copy_status = http->get_response_header(constants::header_ms_copy_status); - properties.last_modified = curl_getdate(http->get_response_header(constants::header_last_modified).c_str(), NULL); + properties.last_modified = curl_getdate(http->get_response_header(constants::header_last_modified).c_str(), nullptr); std::string::size_type sz = 0; std::string contentLength = http->get_response_header(constants::header_content_length); if (contentLength.length() > 0) From 9abb3b9c3eb8152883b8e5fb874e69e2f6f813a0 Mon Sep 17 00:00:00 2001 From: Alexandre Baiao Date: Mon, 23 Dec 2019 09:45:55 -0300 Subject: [PATCH 4/7] Improving ctor chain --- include/blob/blob_client.h | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/include/blob/blob_client.h b/include/blob/blob_client.h index 4f0f606..73b6def 100644 --- a/include/blob/blob_client.h +++ b/include/blob/blob_client.h @@ -33,32 +33,30 @@ namespace azure { namespace storage_lite { /// /// An existing object. /// An int value indicates the maximum concurrency expected during execute requests against the service. - blob_client(std::shared_ptr account, int max_concurrency) - : m_account(account) + /// A string value with absolute path to CA bundle location. + blob_client(std::shared_ptr account, int max_concurrency, const std::string& ca_path = "") + : blob_client(account, + std::make_shared(std::make_shared(), std::make_shared()), + (ca_path.empty() ? std::make_shared(max_concurrency) + : std::make_shared(max_concurrency, ca_path))) { - m_context = std::make_shared(std::make_shared(), std::make_shared()); - m_client = std::make_shared(max_concurrency); } /// /// Initializes a new instance of the class. /// /// An existing object. + /// A context of execution defining how to parser and the retry policy class. /// An int value indicates the maximum concurrency expected during execute requests against the service. /// A string value with absolute path to CA bundle location. - blob_client(std::shared_ptr account, int max_concurrency, const std::string& ca_path) - : m_account(account) - { - m_context = std::make_shared(std::make_shared(), std::make_shared()); - m_client = std::make_shared(max_concurrency, ca_path); - } - blob_client(std::shared_ptr account, std::shared_ptr context, - std::shared_ptr client) - : m_client(client), - m_account(account), - m_context(context) + int max_concurrency, + const std::string ca_path = "") + : blob_client(account, + context, + (ca_path.empty() ? std::make_shared(max_concurrency) + : std::make_shared(max_concurrency, ca_path))) { } @@ -330,6 +328,15 @@ namespace azure { namespace storage_lite { AZURE_STORAGE_API std::future> start_copy(const std::string &sourceContainer, const std::string &sourceBlob, const std::string &destContainer, const std::string &destBlob); private: + blob_client(std::shared_ptr account, + std::shared_ptr context, + std::shared_ptr client) + : m_client(client), + m_account(account), + m_context(context) + { + } + std::shared_ptr m_client; std::shared_ptr m_account; std::shared_ptr m_context; From 971ba3b2f66b9ec2f15092cd2d99d73c655739b0 Mon Sep 17 00:00:00 2001 From: Alexandre Baiao Date: Mon, 23 Dec 2019 10:03:19 -0300 Subject: [PATCH 5/7] Adding constants in constants.dat --- include/constants.dat | 3 +++ include/constants.h | 4 +++- include/retry.h | 4 +++- src/constants.cpp | 2 ++ 4 files changed, 11 insertions(+), 2 deletions(-) diff --git a/include/constants.dat b/include/constants.dat index 37fe556..38ce402 100644 --- a/include/constants.dat +++ b/include/constants.dat @@ -118,6 +118,9 @@ DAT(header_value_storage_version, "2018-11-09") DAT(header_value_user_agent, "azure-storage-cpplite/0.1.0") +DAT_TYPED(int, maximum_retries, 3) +DAT_TYPED(int, base_timeout, 10) + DAT(date_format_rfc_1123, "%a, %d %b %Y %H:%M:%S GMT") DAT(date_format_iso_8601, "%Y-%m-%dT%H:%M:%SZ") diff --git a/include/constants.h b/include/constants.h index ec139d8..8f1f212 100644 --- a/include/constants.h +++ b/include/constants.h @@ -5,7 +5,9 @@ namespace azure { namespace storage_lite { namespace constants { #define DAT(x, y) extern AZURE_STORAGE_API const char *x; const int x ## _size{ sizeof(y) / sizeof(char) - 1 }; +#define DAT_TYPED(type, name, value) extern AZURE_STORAGE_API const type name; #include "constants.dat" #undef DAT +#undef DAT_TYPED -}}} // azure::storage_lite::constants \ No newline at end of file +}}} // azure::storage_lite::constants diff --git a/include/retry.h b/include/retry.h index 6d7ac83..8dbfd52 100644 --- a/include/retry.h +++ b/include/retry.h @@ -6,6 +6,7 @@ #include "storage_EXPORTS.h" +#include "constants.h" #include "http_base.h" #include "utility.h" @@ -79,7 +80,8 @@ namespace azure { namespace storage_lite { { public: - retry_policy(const int maximum_retries = 3, const std::chrono::seconds base_timeout = std::chrono::seconds{10}) + retry_policy(const int maximum_retries = azure::storage_lite::constants::maximum_retries, + const std::chrono::seconds base_timeout = std::chrono::seconds{azure::storage_lite::constants::base_timeout}) : m_maximum_retries(maximum_retries), m_base_timeout(base_timeout) { diff --git a/src/constants.cpp b/src/constants.cpp index 5f4e7e0..b5f1f0a 100644 --- a/src/constants.cpp +++ b/src/constants.cpp @@ -3,7 +3,9 @@ namespace azure { namespace storage_lite { namespace constants { #define DAT(x, y) const char *x{ y }; +#define DAT_TYPED(type, name, value) const type name{value}; #include "constants.dat" #undef DAT +#undef DAT_TYPED }}} // azure::storage_lite From 39ed702203d635c70b285f9cba47f05c4b4afb0e Mon Sep 17 00:00:00 2001 From: Alexandre Baiao Date: Mon, 23 Dec 2019 10:39:35 -0300 Subject: [PATCH 6/7] Removing code duplication --- src/blob/blob_client.cpp | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/src/blob/blob_client.cpp b/src/blob/blob_client.cpp index 11542af..40ab0d5 100644 --- a/src/blob/blob_client.cpp +++ b/src/blob/blob_client.cpp @@ -51,6 +51,20 @@ ssize_t get_length_from_content_range(const std::string &header) return result; } +std::vector> filterHeaders(const std::map& headers, const std::string& value) +{ + std::vector> filteredHeaders; + filteredHeaders.reserve(headers.size()); + for (const auto& header : headers) + { + if (header.first.find(value) == 0) + { + filteredHeaders.emplace_back(header.first.substr(value.size()), header.second); + } + } + return filteredHeaders; +} + } // noname namespace storage_outcome blob_client::get_chunk_to_stream_sync(const std::string &container, const std::string &blob, unsigned long long offset, unsigned long long size, std::ostream &os) @@ -196,15 +210,7 @@ std::future> blob_client::get_container_prop container_property properties(true); properties.etag = http->get_response_header(constants::header_etag); - const auto& headers = http->get_response_headers(); - properties.metadata.reserve(headers.size()); - for (const auto& header : headers) - { - if (header.first.find(constants::header_ms_meta_prefix) == 0) - { - properties.metadata.emplace_back(header.first.substr(constants::header_ms_meta_prefix_size), header.second); - } - } + properties.metadata = filterHeaders(http->get_response_headers(), constants::header_ms_meta_prefix); return storage_outcome(properties); } else @@ -284,16 +290,7 @@ std::future> blob_client::get_blob_properties(con properties.size = std::stoull(contentLength, &sz, 0); } - const auto& headers = http->get_response_headers(); - properties.metadata.reserve(headers.size()); - for (const auto& header : headers) - { - if (header.first.find(constants::header_ms_meta_prefix) == 0) - { - // We need to strip ten characters from the front of the key to account for "x-ms-meta-". - properties.metadata.emplace_back(header.first.substr(constants::header_ms_meta_prefix_size), header.second); - } - } + properties.metadata = filterHeaders(http->get_response_headers(), constants::header_ms_meta_prefix); return storage_outcome(properties); } else From 72a7b4668ea677c62c4af85db7c364227b9160b0 Mon Sep 17 00:00:00 2001 From: Alexandre Baiao Date: Thu, 26 Dec 2019 10:57:51 -0300 Subject: [PATCH 7/7] Moving filter_headers method. This method has potential 5 places to be reused accordingly with @katmsft and should be placed in a general place accessible by all the code that needs it. --- include/utility.h | 4 ++++ src/blob/blob_client.cpp | 18 ++---------------- src/utility.cpp | 15 +++++++++++++++ 3 files changed, 21 insertions(+), 16 deletions(-) diff --git a/include/utility.h b/include/utility.h index 2b26fb1..7103d41 100644 --- a/include/utility.h +++ b/include/utility.h @@ -2,6 +2,8 @@ #include #include +#include +#include #include "storage_EXPORTS.h" @@ -28,6 +30,8 @@ namespace azure { namespace storage_lite { AZURE_STORAGE_API std::string get_http_verb(http_base::http_method method); + AZURE_STORAGE_API std::vector> filter_headers(const std::map& headers, const std::string& value); + inline void add_optional_query(storage_url &url, const std::string &name, unsigned int value) { if (value > 0) diff --git a/src/blob/blob_client.cpp b/src/blob/blob_client.cpp index 40ab0d5..d5c8b79 100644 --- a/src/blob/blob_client.cpp +++ b/src/blob/blob_client.cpp @@ -51,20 +51,6 @@ ssize_t get_length_from_content_range(const std::string &header) return result; } -std::vector> filterHeaders(const std::map& headers, const std::string& value) -{ - std::vector> filteredHeaders; - filteredHeaders.reserve(headers.size()); - for (const auto& header : headers) - { - if (header.first.find(value) == 0) - { - filteredHeaders.emplace_back(header.first.substr(value.size()), header.second); - } - } - return filteredHeaders; -} - } // noname namespace storage_outcome blob_client::get_chunk_to_stream_sync(const std::string &container, const std::string &blob, unsigned long long offset, unsigned long long size, std::ostream &os) @@ -210,7 +196,7 @@ std::future> blob_client::get_container_prop container_property properties(true); properties.etag = http->get_response_header(constants::header_etag); - properties.metadata = filterHeaders(http->get_response_headers(), constants::header_ms_meta_prefix); + properties.metadata = filter_headers(http->get_response_headers(), constants::header_ms_meta_prefix); return storage_outcome(properties); } else @@ -290,7 +276,7 @@ std::future> blob_client::get_blob_properties(con properties.size = std::stoull(contentLength, &sz, 0); } - properties.metadata = filterHeaders(http->get_response_headers(), constants::header_ms_meta_prefix); + properties.metadata = filter_headers(http->get_response_headers(), constants::header_ms_meta_prefix); return storage_outcome(properties); } else diff --git a/src/utility.cpp b/src/utility.cpp index 95dcdfc..8c19ab4 100644 --- a/src/utility.cpp +++ b/src/utility.cpp @@ -123,6 +123,21 @@ namespace azure { namespace storage_lite { return std::string(); } + std::vector> filter_headers(const std::map& headers, + const std::string& value) + { + std::vector> filteredHeaders; + filteredHeaders.reserve(headers.size()); + for (const auto& header : headers) + { + if (header.first.find(value) == 0) + { + filteredHeaders.emplace_back(header.first.substr(value.size()), header.second); + } + } + return filteredHeaders; + } + void add_access_condition_headers(http_base &h, storage_headers &headers, const blob_request_base &r) { if (!r.if_modified_since().empty())