-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
passes ContainerURL to getBlobURL for Azure #4607
Conversation
Signed-off-by: Wiard van Rij <wiard@outlook.com>
From my perspective this solves the major throttling issues, as it reuses the same credential instance for all Azure API requests. |
Signed-off-by: Wiard van Rij <wiard@outlook.com>
Signed-off-by: Wiard van Rij <wiard@outlook.com>
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.
LGTM, just one nit. Thanks!
I wonder if need path release. 0.23 is soon there!
return blob.BlockBlobURL{}, err | ||
} | ||
return c.NewBlockBlobURL(blobName), nil | ||
func getBlobURL(blobName string, c blob.ContainerURL) blob.BlockBlobURL { |
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.
Sounds like not useful function tbh. Worth to inline?
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 tried to do minimal changes in the 'structure'. There are multiple things which seem not useful or very vague. I've considered just removing this function since the 'logic' got removed. However it has 5 references and my intent is to leave many things 'as is'. Once I got my Azure stack back, I'm eager to probably rewrite a lot more for the Azure package. Because it would not surprise me if we are doing things really inefficient or vague.
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.
Not a biggie, but I don't think changing one function/method is a big refactor (:
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.
Not a biggie, but I don't think changing one function/method is a big refactor (:
No it isn't but it gives me some context on 'how it is/was'. My intent is to remove the entire helpers.go
or at least make it more logical.
Signed-off-by: Wiard van Rij wiard@outlook.com
ContainerURL
containsclient containerClient
and at the moment each call viagetBlobURL
would require a new pass towards the authentication part.Pretty sure this worked "fine" for using auth tokens which might have different rate limits(?). Yet this might hit rate limits as per #4605
To be honest the code still feels a bit odd, but hopefully this change actually works and makes sense.
Verification
I could not test this, as I ran out of Azure credits (: but Philip Laine can do this as per discussion on Slack :)
Relates to #4605