-
Notifications
You must be signed in to change notification settings - Fork 14
Latest 5.0 API #221
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.
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
Latest 5.0 API #221
Conversation
CSDK-268: 4.1 API
CSDK-171: Removing ChunkClientProcessingOrderGuarantee from request payload body
src/ds3_bool.h
Outdated
| #endif | ||
|
|
||
| // For windows DLL symbol exports. | ||
| #ifdef _WIN32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this LIBRARY_EXPORTS moved or was it copied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copied to be the same as other .h files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only define it once
| } | ||
| void ds3_request_set_auto_compaction_threshold_in_bytes(const ds3_request* request, const uint64_t value) { | ||
| _set_query_param_uint64_t(request, "auto_compaction_threshold_in_bytes", value); | ||
| void ds3_request_set_auto_compaction_threshold(const ds3_request* request, const int value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was the uint64_t changed to an int?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because the API changed. It went from threshold in bytes to just threshold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meaning a percent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume based on Carl's comments, but the contract doesn't specify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess my concern was that we went from something that is non-negative, to something that could be negative.
|
|
||
| return _internal_request_dispatcher(client, request, NULL, NULL, NULL, NULL, NULL); | ||
| error = _init_request_payload(request, &send_buff, ID_LIST); | ||
| if (error != NULL) return error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we get here, what happens to send_buff? Do we know if the data structure allocated for it is freed in the event of an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
send_buff is not dynamically allocated in the request handler. The contents only need to be freed if the _init_request_payload succeeds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
| } | ||
|
|
||
| return _internal_request_dispatcher(client, request, NULL, NULL, NULL, NULL, NULL); | ||
| error = _init_request_payload(request, &send_buff, ID_LIST); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here about send_buff
| if (checksum_str == NULL || checksum_str->value == NULL) { | ||
| return NULL; | ||
| } | ||
| ds3_checksum_type* checksum_type = g_new0(ds3_checksum_type, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we allocating space for this? Why wouldn't we just return DS3_CHECKSUM_TYPE_CRC_32 directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because the header may not exist, so a nullable pointer represents the none value. I could change it to have a default of CRC32 if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we default to a NONE checksum type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no NONE checksum type defined in the contract. I'd have to special case in the value if you want it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it. Leave it for now then. I was hoping we could get rid of an extra allocation that seemed unneeded.
src/ds3_uint64_string_map.h
Outdated
| #endif | ||
|
|
||
| // For windows DLL symbol exports. | ||
| #ifdef _WIN32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be duplicated again. Is there a single place where we can define this and then include it?
src/ds3_library_exports.h
Outdated
| @@ -0,0 +1,19 @@ | |||
| // | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use the correct copyright details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have looked more closely at what CLion automatically created :p
Changes
4_1_autogenbranch to import all files and reconfiguration done for supporting checksum header parsing.