Exposing new constructor to blob_client. #54
Conversation
src/blob/blob_client.cpp
Outdated
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
include/retry.h
Outdated
| { | ||
| public: | ||
|
|
||
| retry_policy(const int maximum_retries = 3, const std::chrono::seconds base_timeout = std::chrono::seconds{10}) |
There was a problem hiding this comment.
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.
include/blob/blob_client.h
Outdated
|
|
||
| blob_client(std::shared_ptr<storage_account> account, | ||
| std::shared_ptr<executor_context> context, | ||
| std::shared_ptr<CurlEasyClient> client) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
katmsft
left a comment
katmsft
left a comment
There was a problem hiding this comment.
Thanks very much for the contribution! Exception for the last minor comments other parts looks fine by me. Please resolve it and we can proceed to validate/merge the PR :)
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.
3 participants
Exposing a new constructor to blob_client in order to be able to allow a dependency injection pattern in the class.
Also, simplifying the retry logic.
Modernizing some parts of the code.
And adding const correctness in the container_property class.