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
storage: update azure blobstore client to use new sdk #5302
storage: update azure blobstore client to use new sdk #5302
Conversation
@@ -288,7 +296,7 @@ replace github.com/sercand/kuberesolver => github.com/sercand/kuberesolver v2.4. | |||
|
|||
replace github.com/hpcloud/tail => github.com/grafana/tail v0.0.0-20201004203643-7aa4e4a91f03 | |||
|
|||
replace github.com/Azure/azure-sdk-for-go => github.com/Azure/azure-sdk-for-go v36.2.0+incompatible | |||
replace github.com/Azure/azure-sdk-for-go/sdk/storage/azblob => github.com/MasslessParticle/azure-sdk-for-go/sdk/storage/azblob v0.2.1-0.20220131200443-9793e6f0cc65 |
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 an open PR to msft from this fork that contains a deadlock fix. Point to it until the PR is merged
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.
Can you share the PR ?
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.
@@ -37,23 +34,18 @@ const ( | |||
|
|||
var ( | |||
supportedEnvironments = []string{azureGlobal, azureChinaCloud, azureGermanCloud, azureUSGovernment} | |||
noClientKey = azblob.ClientProvidedKeyOptions{} | |||
endpoints = map[string]struct{ blobURLFmt, containerURLFmt string }{ | |||
endpoints = map[string]struct{ blobURLFmt string }{ |
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.
sdk handles the container url now. We just have to care about which account we're signing into
@@ -80,77 +72,6 @@ var ( | |||
} | |||
) | |||
|
|||
// BlobStorageConfig defines the configurable flags that can be defined when using azure blob storage. | |||
type BlobStorageConfig struct { |
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.
Moved this so the storage_client
file only contains the client itself
|
||
// Process the blobs returned in this result segment (if the segment is empty, the loop body won't execute) | ||
for _, blobInfo := range listBlob.Segment.BlobItems { | ||
err := instrument.CollectedRequest(ctx, "azure.List", instrument.NewHistogramCollector(b.metrics.requestDuration), instrument.ErrorCode, func(ctx context.Context) 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.
The new API means that this instrumentation is now for how long it takes to list all the blobs rather than each page of blobs
edfb2a4
to
700f455
Compare
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
Well done
700f455
to
b0c4098
Compare
* Update azure blobstore client to use new sdk * changelog * fix hardcoded maxretries * linter * more linting * Fix flaky test
This PR updates the Azure Blobstore client to use the new
azure-sdk-for-go/sdk/storage
package. It basically amounts to a rewrite.The huge number of files changed is because of vendored dependencies.
The files to pay attention to are all in the
storage/chunk/azure
package. I also took this as an opportunity to refactor the Azure metrics and config to their own files soblob_storage_client.go
is where the meat of this PR liesChecklist
CHANGELOG.md
about the changes.