CPPLite initial commit#1
Conversation
|
@JasonYang-MSFT , can you CR? |
| virtual iterator_base end() = 0; | ||
| }; | ||
|
|
||
| class my_iterator : public my_iterator_base { |
There was a problem hiding this comment.
my_iterator [](start = 14, length = 11)
Check whether it is used or not. If not, we can remove it, otherwise we need a better naming.
| DAT(http_post, "POST") | ||
| DAT(http_put, "PUT") | ||
|
|
||
| DAT(default_endpoint_suffix, ".core.windows.net") |
There was a problem hiding this comment.
default_endpoint_suffix [](start = 4, length = 23)
Besides default suffix, does other environment supported or not?
There was a problem hiding this comment.
Yes, it requires the user to specify the endpoint and it has to be the form of "account.blob.endpoint".
|
|
||
| //AZURE_STORAGE_API void build_request(const storage_account &a, const get_blob_request_base &r, http_base &h); | ||
|
|
||
| class blob_property |
There was a problem hiding this comment.
blob_property [](start = 14, length = 13)
We should move blob_property to a common header file instead of a request_base.h
There was a problem hiding this comment.
This item is recorded, and can be fix in the future. This refinement is not going to break the customer and is not critical enough to address in the initial release.
| // blob_type m_type; | ||
| // azure::storage::lease_status m_lease_status; | ||
| // azure::storage::lease_state m_lease_state; | ||
| // azure::storage::lease_duration m_lease_duration; |
There was a problem hiding this comment.
Those value should be necessary properties for blob, right?
There was a problem hiding this comment.
Yes, but the logic of parsing them is not yet finished, and it will be done if requested by customer.
| namespace microsoft_azure { | ||
| namespace storage { | ||
|
|
||
| void append_block_request_base::build_request(const storage_account &a, http_base &h) const { |
There was a problem hiding this comment.
a [](start = 77, length = 1)
I think we should use more meaningful names than 'a', 'h', 'r'.
There was a problem hiding this comment.
This is a refinement that can be done in the future, which does not introduce breaking changes.
| url.append_path(r.container()).append_path(r.blob()); | ||
|
|
||
| url.add_query(constants::query_comp, constants::query_comp_appendblock); | ||
| add_optional_query(url, constants::query_timeout, r.timeout()); |
There was a problem hiding this comment.
r.timeout() [](start = 62, length = 11)
timeout() is always 0. So there is no way to set request time out?
There was a problem hiding this comment.
Yes, it is already recorded in the missing feature.
| add_optional_content_md5(h, headers, r.content_md5()); | ||
| add_access_condition_headers(h, headers, r); | ||
|
|
||
| add_ms_header(h, headers, constants::header_ms_client_request_id, r.ms_client_request_id(), true); |
There was a problem hiding this comment.
ms_client_request_id [](start = 80, length = 20)
request id always empty too?
There was a problem hiding this comment.
Yes, it is already recorded in the missing feature.
| } while (!marker.empty()); | ||
|
|
||
| return results; | ||
| }*/ |
There was a problem hiding this comment.
This code is marked out, we can remove it.
| h.set_url(url.to_string()); | ||
|
|
||
| storage_headers headers; | ||
| //add_access_condition_headers(h, headers, r); |
There was a problem hiding this comment.
//add_access_condition_headers(h, headers, r); [](start = 12, length = 46)
Is access condition in scope?
If it is in scope but not ready yet, we can add a TODO flag here.
There was a problem hiding this comment.
Nope, it is currently not in scope, it is recorded in the missing feature.
… list container, renamed to list_container_segmented.
| class create_container_request_base : public blob_request_base | ||
| { | ||
| public: | ||
| enum class blob_public_access { |
There was a problem hiding this comment.
{ [](start = 37, length = 2)
'{'
|
|
||
| virtual std::string container() const = 0; | ||
|
|
||
| virtual blob_public_access ms_blob_public_access() const { |
There was a problem hiding this comment.
{ [](start = 64, length = 2)
'{'
| return blob_public_access::unspecified; | ||
| } | ||
|
|
||
| //virtual std::map<std::string, std::string> ms_meta() const {}; |
There was a problem hiding this comment.
//virtual std::map<std::string, std::string> ms_meta() const {}; [](start = 8, length = 64)
Remove it or make a TODO here.
| *.dylib | ||
|
|
||
| # Executables | ||
| *.exe |
There was a problem hiding this comment.
Will there be .exe if this change is to support Linux only?
There was a problem hiding this comment.
You are right, this will be modified.
|
|
||
| project(azurestoragelite) | ||
|
|
||
| option(BUILD_TESTS "Build test codes" OFF) |
There was a problem hiding this comment.
Will this make BUILD_TESTS always OFF?
There was a problem hiding this comment.
No, when user specifies -DBUILD_TESTS=true, tests will be build.
| "inheritEnvironments": [ "linux-x64" ], | ||
| "variables": [ | ||
| { | ||
| "name": "BUILD_TESTS", |
There was a problem hiding this comment.
Will this overwrite option(BUILD_TESTS "Build test codes" OFF)?
There was a problem hiding this comment.
Yes, this is develop only environment, so this should always build tests.
| # Contributing | ||
| ## About | ||
|
|
||
| The Azure Storage Client Library for C++ allows you to build applications against Microsoft Azure Storage's blob service. This is a minimum dependency version that provide basic object storage. For an overview of Azure Storage, see [Introduction to Microsoft Azure Storage](http://azure.microsoft.com/en-us/documentation/articles/storage-introduction/). |
There was a problem hiding this comment.
Azure Storage Client Library for C++ (Lite)?
There was a problem hiding this comment.
Give a quick link here for customer wants to use tqf and other blob's missing feature.
There was a problem hiding this comment.
Both comments are valid, this will be modified in the next commit.
| azure::storage_lite::blob_client client(storage_account, connection_count); | ||
|
|
||
| // Start using | ||
| auto outcome = client.create_container("YOUR_CONTAINER_NAME"); |
There was a problem hiding this comment.
If no wait happened here, the sample program will exit, maybe before sending requests to the server.
There was a problem hiding this comment.
True, added .get() to get the result.
| class storage_request_base | ||
| { | ||
| public: | ||
| virtual std::string ms_client_request_id() const { return std::string(); } |
There was a problem hiding this comment.
It is better to create request id for each request for debug purpose.
There was a problem hiding this comment.
This is a valid improvement, but is beyond the scope of this release. Added todo tag to track.
| url.add_optional_query(constant::query_timeout, (r.timeout() != std::numeric_limits<unsigned long long>::max() ? std::to_string(r.timeout()) : std::string())); | ||
| h.set_url(url.to_string()); | ||
|
|
||
| details::add_optional_header(h, constant::header_content_encoding, r.content_encoding()); |
There was a problem hiding this comment.
These headers are not needed for get blob metadata, right?
There was a problem hiding this comment.
That is true, this should be used with set blob metadata or set blob properties. Added todo tag to move it when implementing those APIs.
| url.add_optional_query(constant::query_timeout, (r.timeout() != std::numeric_limits<unsigned long long>::max() ? std::to_string(r.timeout()) : std::string())); | ||
| h.set_url(url.to_string()); | ||
|
|
||
| details::add_optional_header(h, constant::header_content_encoding, r.content_encoding()); |
There was a problem hiding this comment.
Please double check the headers and create items for mising headers.
For example, copy source, copy progress, incremental copy.
There was a problem hiding this comment.
As discussed offline, this is in todo list, and should be removed from cmakelist.
| namespace storage_lite { | ||
| namespace experimental { | ||
|
|
||
| struct query_entities_request_base : table_request_base { |
There was a problem hiding this comment.
If table is not in the scope, it is better to remove from this release.
There was a problem hiding this comment.
As discussed offline, this is in todo list, and should be removed from cmakelist.
| template<typename Request, typename Http> | ||
| struct storage_request_processor<Request, Http, std::enable_if_t<std::is_base_of<set_blob_metadata_request, std::remove_reference_t<Request>>::value>> { | ||
| static inline void process(Request &&r, Http &&h) { | ||
| //static_assert(std::is_same<decltype(r.use_https()), decltype(r.get_blob_request::use_https())>::value, "get_blob_request::use_https != Request::use_https"); |
There was a problem hiding this comment.
As discussed offline, this is in todo list, and should be removed from cmakelist.
| }; | ||
|
|
||
| template<> | ||
| inline list_containers_response xml_parser_base::parse_response<list_containers_response>(const std::string &xml) const\ |
|
|
||
| namespace azure { namespace storage_lite { | ||
|
|
||
| namespace { |
There was a problem hiding this comment.
As discussed offline, this is to avoid namespace clash for helper functions.
|
|
||
| http->set_output_stream(storage_ostream(os)); | ||
|
|
||
| const auto response = async_executor<void>::submit(m_account, request, http, m_context).get(); |
There was a problem hiding this comment.
This is where the request changed from async to sync then return with async.
There was a problem hiding this comment.
Sure, but there is no plan to change it at the moment. The initial release will leave it as is.
|
|
||
| auto request = std::make_shared<get_container_property_request>(container); | ||
|
|
||
| auto response = async_executor<void>::submit(m_account, request, http, m_context).get(); |
There was a problem hiding this comment.
Here, async to sync too. Think about to add the post process as a function.
There was a problem hiding this comment.
This is also noted as TODO and to be refined in the future.
| return storage_outcome<container_property>(containerProperty); | ||
| } | ||
|
|
||
| std::future<storage_outcome<list_containers_response>> blob_client::list_containers_segmented(const std::string &prefix, const std::string& continuation_token, const int max_result, bool include_metadata) |
There was a problem hiding this comment.
list_containers_response, the name should align with list blobs segmented.
| /// </summary> | ||
| /// <param name="account">An existing <see cref="azure::storage_lite::storage_account" /> object.</param> | ||
| /// <param name="size">An int value indicates the maximum concurrency expected during execute requests against the service.</param> | ||
| blob_client(std::shared_ptr<storage_account> account, int size) |
There was a problem hiding this comment.
size is actually the maximun concurrency value, consider to change the name. Also you can try to add a default value.
There was a problem hiding this comment.
I don't have a good proposal for the optimized concurrency count, but I have already changed the naming. Any thoughts?
| { | ||
| const auto &r = *this; | ||
|
|
||
| h.set_absolute_timeout(30L); |
There was a problem hiding this comment.
Those timeouts are magic values, we should find a way to allow setting max execution time, request time out
There was a problem hiding this comment.
Added todo for future enhancement.
|
|
||
| size_t CurlEasyRequest::header_callback(char *buffer, size_t size, size_t nitems, void *userdata) | ||
| { | ||
| CurlEasyRequest::MY_TYPE *p = static_cast<CurlEasyRequest::MY_TYPE *>(userdata); |
There was a problem hiding this comment.
Try to have a better name than "MY_TYPE".
|
|
||
| inline void add_ms_header(http_base &h, storage_headers &headers, const std::string &name, const std::string &value, bool optional = false) | ||
| { | ||
| if (!optional || !value.empty()) { |
There was a problem hiding this comment.
If a required header is missing, it will still proceed?
There was a problem hiding this comment.
If a required header is missing, it means that the header must be added with an empty value. The function will proceed to add it to the headers and proceed.
|
|
||
| inline void add_ms_header(http_base &h, storage_headers &headers, const std::string &name, unsigned long long value, bool optional = false) | ||
| { | ||
| if (!optional || !value) { |
There was a problem hiding this comment.
!value is not align with other place I saw the definition of not set unsigned long long value. In some place, it is using max value of unsigned long long.
There was a problem hiding this comment.
True, now changed it to compare with max value of unsigned long long.
| string_to_sign.append(headers.content_length).append("\n"); | ||
| string_to_sign.append(headers.content_md5).append("\n"); | ||
| string_to_sign.append(headers.content_type).append("\n"); | ||
| string_to_sign.append("\n"); // Date |
There was a problem hiding this comment.
Why Date is not in string to sign?
There was a problem hiding this comment.
We are only using x-ms-date, so Date header will always be empty.
JasonYang-MSFT
left a comment
JasonYang-MSFT
left a comment
There was a problem hiding this comment.
As discussed, first release is mostly release as it is in blob fuse. As long as we mark out the missing features, covered basic functionalities, fixed the coding style and well documented, I think it is OK to merge.
3 participants
No description provided.