-
Notifications
You must be signed in to change notification settings - Fork 530
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
Upgrade Azure SDK from azure-storage-blob-go
to azure-sdk-for-go
#2882
Conversation
This pull request is not ready for review, but anyone interested is welcome to take a look at the work in progress. |
azure-storage-blob-go
to azure-sdk-for-go
azure-storage-blob-go
](https://github.com/Azure/azure-storage-blob-go) to azure-sdk-for-go
azure-storage-blob-go
](https://github.com/Azure/azure-storage-blob-go) to azure-sdk-for-go
azure-storage-blob-go
to azure-sdk-for-go
Tests are passing locally, and this should be ready for review. I can't run the pull request workflows myself, and am thus unable to verify that the CI tests will also pass. |
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.
So far so good. I'm going to build this image and run in our azure dev environment. Will report back if there are issues.
@joe-elliott Thanks for your review! I've got some internal tasks I need to prioritise, after which I will take a look at this again. ETA a couple of days. |
@joe-elliott Comments have been addressed. Are you able to take another look? |
@joe-elliott Thank you for the review. All comments have been addressed. Can you take another look? 👀 |
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.
Amazing contribution. Thank you 🙏
Looks like there's a changelog conflict. Clean that up and we'll merge!
Conflicts have been fixed and this should be ready for a merge! |
Looks like a test is failing, I'm taking a look. |
Hmm, looks like the |
@LasseHels If there is a failing test that you don't think is a result of your change, always feel free to re-run the failed job if you have the permission. Thanks for the PR. |
I see now that the test failed on |
@LasseHels Have you been able to run this code change in your environment? We continue to see this test failure pop up intermittently and wondering about next steps. It would help to know if we believe this was only a test failure, or if the change is also experiencing issues in an Azure environment. |
|
The failure looks specific to the Write test and a large buffer to force the multiple calls. #2948 to work around the test. Would still like to know how this is performing in a real environment. |
@zalegrala We are not running this change in our own environments. As such, I can't say whether the test failure is a false or true positive. I might be able to spend some investigation time over the next couple of days to try to figure out what's going on. If you deem this to be an urgent issue, then you may be better off not relying on me, as I can't guarantee my availability. |
@zalegrala I spent a couple of hours looking into the issue and got nowhere. I can reproduce it locally, but I don't know the root cause yet. I can't tell if the issue is in our implementation of the Azure SDK, our test, the SDK itself or somewhere else. I also see we created an issue in the SDK repository, will link that here for visibility: Azure/azure-sdk-for-go#21555 |
Thanks for the info @LasseHels. I also spent some time researching and wasn't able to nail it down. I've got #2952 up to bring back the old code as v1, and use the new code as v2 with a config option to switch. This will allow us to gain some confidence as we move forward and give time for the Azure folks to dig into that issue. Keep an eye on the changelog and docs for the final name of the config option in case it changes before merge. |
What this PR does:
This pull request updates Tempo's
azure
package to use the azure-sdk-for-go SDK instead of the deprecated azure-storage-blob-go SDK.This pull request aims to be fully backwards compatible, and is mainly a refactor. At least one functional change is included:
See https://github.com/Azure/azure-sdk-for-go/blob/main/sdk/storage/azblob/migrationguide.md for a non-exhaustive list of the differences between the two SDKs.
Which issue(s) this PR fixes:
Fixes #2129
Fixes #2835
Checklist
main
.Documentation added- I'm not sure documentation updates are necessary for this change.CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]