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

Jump to bottom

Specify very large block size to force put blob instead of stage block/put block list #2561

Merged
merged 9 commits into from
Feb 13, 2024
Merged

Specify very large block size to force put blob instead of stage block/put block list #2561

merged 9 commits into from
Feb 13, 2024

Conversation

gapra-msft
Copy link
Member

No description provided.

@gapra-msft gapra-msft changed the title Code changes for setting put blob size Specify very large block size to force put blob instead of stage block/put block list Jan 31, 2024
@gapra-msft gapra-msft marked this pull request as ready for review January 31, 2024 23:01
@vibhansa-msft vibhansa-msft added this to the 10.24 milestone Feb 1, 2024
adreed-msft
adreed-msft approved these changes Feb 12, 2024
View reviewed changes
Copy link
Member

@adreed-msft adreed-msft left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Work is mostly fine, I have some comments regarding defaults, overriding, etc.

cmd/copy.go Show resolved Hide resolved
ste/mgr-JobPartTransferMgr.go Show resolved Hide resolved
ste/mgr-JobPartTransferMgr.go Show resolved Hide resolved
ste/sender-blockBlob.go Show resolved Hide resolved
adreed-msft
adreed-msft approved these changes Feb 12, 2024 •
edited
Loading
View reviewed changes
@gapra-msft
Copy link
Member Author

Didn't see this prior to my last comment. Does it really make sense to override the user's input here instead of indicating it is nonsensical? If we do override the user's input, we should probably log that we did, and make it generally apparent that we are doing something intentionally different, otherwise it may be looked at as a bug.

I was mostly copying the pattern of how we deal with BlockBlobBLockSize. We should probably log that we updated it, you're right. I will fix that for both BlockBlobBlockSize and this new variable

@gapra-msft
Copy link
Member Author

Secondarily, DefaultPutBlobSize is unused.

You're right. I think I was planning to use DefaultPutBlobSize then decided it's probably better to use the block size so that customers don't experience any "breaking changes" from this PR. I will get rid of the DefaultPutBLobSize var and update the comment.

@gapra-msft gapra-msft merged commit 06b1fb3 into main Feb 13, 2024
17 checks passed
@gapra-msft gapra-msft deleted the gapra/maxPutBlobSize branch February 13, 2024 02:42
@BrewTestBot BrewTestBot mentioned this pull request Mar 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vibhansa-msft vibhansa-msft vibhansa-msft left review comments

@adreed-msft adreed-msft adreed-msft approved these changes

@siminsavani-msft siminsavani-msft siminsavani-msft approved these changes

@nakulkar-msft nakulkar-msft Awaiting requested review from nakulkar-msft

@tasherif-msft tasherif-msft Awaiting requested review from tasherif-msft

@seanmcc-msft seanmcc-msft Awaiting requested review from seanmcc-msft seanmcc-msft is a code owner

@pranavmalik-msft pranavmalik-msft Awaiting requested review from pranavmalik-msft

Assignees

@siminsavani-msft siminsavani-msft

@pranavmalik-msft pranavmalik-msft

Labels
pr-ready-for-review
Projects
None yet
Milestone
10.24
Development

Successfully merging this pull request may close these issues.
5 participants
@gapra-msft @adreed-msft @vibhansa-msft @siminsavani-msft @pranavmalik-msft