Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exposing new constructor to blob_client. #54

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Exposing new constructor to blob_client. #54

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
11ccadb
Fixes in retry
AlexandreBaiao Dec 20, 2019
3ce274e
Some refactoring
AlexandreBaiao Dec 20, 2019
f80fd9e
Const correctness and modernization
AlexandreBaiao Dec 20, 2019
9abb3b9
Improving ctor chain
AlexandreBaiao Dec 23, 2019
971ba3b
Adding constants in constants.dat
AlexandreBaiao Dec 23, 2019
39ed702
Removing code duplication
AlexandreBaiao Dec 23, 2019
72a7b46
Moving filter_headers method.
AlexandreBaiao Dec 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions include/blob/blob_client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,15 @@ namespace azure { namespace storage_lite {
m_client = std::make_shared<CurlEasyClient>(max_concurrency, ca_path);
}

blob_client(std::shared_ptr<storage_account> account,
std::shared_ptr<executor_context> context,
std::shared_ptr<CurlEasyClient> client)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::shared_ptr client [](start = 20, length = 38)

client should be internal logic that is not exposed to customers. Please make the 3rd argument int max_concurrency instead.

Copy link
Author

@AlexandreBaiao AlexandreBaiao Dec 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left the ctor with client for future tests and to join all the ctor logics in to one flow, however it is private so the customer will not use it.
Please recheck and share your thoughts.

: m_client(client),
m_account(account),
m_context(context)
{
}

/// <summary>
/// Gets the curl client used to execute requests.
/// </summary>
Expand Down
2 changes: 1 addition & 1 deletion include/get_container_property_request_base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ namespace azure { namespace storage_lite {
m_valid = valid;
}

bool valid()
bool valid() const
{
return m_valid;
}
Expand Down
32 changes: 22 additions & 10 deletions include/retry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,27 +78,39 @@ 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})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the maximum_retries and base_timeout a reference to a constant variable defined in constant.dat.
This way it is easier for the customer to find where to change them by modifying the code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, please re-check

: 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:
bool can_retry(http_base::http_code code) const
{
return retryable(code);
}

std::chrono::seconds calculate_new_delay(const retry_context &context) const
{
// (1 << N) == 2^N
return ((context.numbers() == 0) ? std::chrono::seconds(0)
: (m_base_timeout * (1 << context.numbers())));
}

const int m_maximum_retries;
const std::chrono::seconds m_base_timeout;
};

}} // azure::storage_lite
Expand Down
22 changes: 12 additions & 10 deletions src/blob/blob_client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ storage_outcome<chunk_property> 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<chunk_property>(property);
}
return storage_outcome<chunk_property>(storage_error(response.error()));
Expand Down Expand Up @@ -196,12 +196,13 @@ std::future<storage_outcome<container_property>> 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);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is duplicated with get_blob_properties, it would be better to abstract the logic and make it a function, for future use in file service.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@katmsft I'm not sure I agree with you. Since this piece of code is processing properties and metadata. Properties for file/blob container/blob might be different. So I think it's also okay to leave it as it is.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The best solution for this is to use a transform_if algorithm, but it is not in the STL.
I agree that we should eliminate duplication, but where do I put this function?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JinmingHu-MSFT You are absolutely right about the properties being different. What I was suggesting was the logic of extracting metadata from the headers. This is universal for blob/container/file/share/directory, potentially there will be 5 places that shares the same code logic.
@AlexandreBaiao I think it would be nice to have it in util.h so that if in the future file service is supported in CPPLite, it can be also called when extracting the metadata.

return storage_outcome<container_property>(properties);
Expand Down Expand Up @@ -275,21 +276,22 @@ std::future<storage_outcome<blob_property>> 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)
{
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<blob_property>(properties);
Expand Down