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.

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

Merged
merged 3 commits into from
Aug 27, 2021
Merged

passes ContainerURL to getBlobURL for Azure #4607

merged 3 commits into from
Aug 27, 2021

Conversation

wiardvanrij
Copy link
Member

@wiardvanrij wiardvanrij commented Aug 26, 2021

Signed-off-by: Wiard van Rij wiard@outlook.com

ContainerURL contains client containerClient and at the moment each call via getBlobURL 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.

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

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

Signed-off-by: Wiard van Rij <wiard@outlook.com>
@phillebaba
Copy link
Contributor

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>
Copy link
Member

@bwplotka bwplotka left a 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 {
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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 (:

Copy link
Member Author

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.

@bwplotka bwplotka merged commit 4b03b54 into thanos-io:main Aug 27, 2021
@phillebaba phillebaba mentioned this pull request Aug 30, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants