generated from amazon-archives/__template_Apache-2.0
-
Notifications
You must be signed in to change notification settings - Fork 50
Dynamic default part size #575
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
Merged
Merged
Changes from 29 commits
Commits
Show all changes
36 commits
Select commit
Hold shift + click to select a range
d804f65
WIP
TingDaoK 5fb1c44
add extract the number of parts from etag
TingDaoK 59c18b2
fix tests
TingDaoK f0ee675
fix build
TingDaoK 772c4a0
apply the dynamic size
TingDaoK a12d2cb
fix the logic for empty file
TingDaoK 3796c1e
fix compiler warning
TingDaoK 350281d
add metrics
TingDaoK cbcc4e5
get part number as well
TingDaoK 3eadf8a
try to fix the flaky test
TingDaoK 2141cfc
let's add a flag and ifdef
TingDaoK d196a83
adding test
TingDaoK 2982e3e
more tests
TingDaoK a58305c
fix some errors
TingDaoK 66424cc
compilers
TingDaoK 1686881
more warnings
TingDaoK 1a334d0
remove my local config
TingDaoK 2e900b4
increase the memory limit so that we are not limited by that
TingDaoK e0fe6d8
in case of retry, let's record the succeed only metrics as well
TingDaoK c7ebc80
Merge branch 'main' into dynamic-default-part-size
TingDaoK bb61f3b
fix the mem metrics logic
TingDaoK 2a4f6bb
well, on 32bit it will default to 1
TingDaoK 72e6976
remove the hard coded 8MiB
TingDaoK e6a395b
add checks for the response to match the request and makes sure the d…
TingDaoK d1fe086
let's see what breaks
TingDaoK 587c3f9
fix the curosr parsing logic
TingDaoK 982fcce
fix couple corner case
TingDaoK c0de491
another concern case
TingDaoK 64a6e5a
well. The mock server was wrong the whole time...
TingDaoK f0199da
merge
TingDaoK af29d4f
fix
TingDaoK 287b507
Apply suggestion from @DmitriyMusatkin
TingDaoK 54d5c38
address comments
TingDaoK 5d7d268
collect part range for auto-range-put as well (#588)
TingDaoK fadd3dc
Merge branch 'main' into dynamic-default-part-size
TingDaoK bd5f363
Buffer pool refactor (#584)
TingDaoK File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -122,8 +122,19 @@ struct aws_s3_request_metrics { | |||
| int error_code; | ||||
| /* Retry attempt. */ | ||||
| uint32_t retry_attempt; | ||||
| /* Is the memory for the request allocated from the buffer pool or not. */ | ||||
| bool memory_allocated_from_pool; | ||||
| } crt_info_metrics; | ||||
|
|
||||
| struct { | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thinking out loud:
we can probably do some of the same sanity checks in upload path as well |
||||
| /* Beginning range of this part. */ | ||||
| uint64_t part_range_start; | ||||
| /* Last byte of this part. */ | ||||
| uint64_t part_range_end; | ||||
| /* Part number that this request refers to. */ | ||||
| uint32_t part_number; | ||||
| } part_info_metrics; | ||||
|
|
||||
| struct aws_ref_count ref_count; | ||||
| }; | ||||
|
|
||||
|
|
||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
any concern adding this def here? Or it's better to keep as a separate one.
pro: Nothing will break.
con: Maybe people built with tests in prod and don't want to have this stub enabled.
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.
do we enable this ourselves when we package java, python, etc...?
probably not a big concern in practice, it add a private api. we already expose a bunch of private apis used by tests only
Uh oh!
There was an error while loading. Please reload this page.
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.
https://github.com/awslabs/aws-crt-python/blob/main/crt/CMakeLists.txt#L26
https://github.com/awslabs/aws-crt-java/blob/main/pom.xml#L185
Yeah, we do set this to off when we build the bindings.
Also, we should not build the C tests when we package for the CRT bindings.