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

Migrate system storage from Azure SDK 1.7 version to 2.5. #79

Closed
pherbel opened this issue Feb 2, 2015 · 14 comments
Closed

Migrate system storage from Azure SDK 1.7 version to 2.5. #79

pherbel opened this issue Feb 2, 2015 · 14 comments

Comments

@pherbel
Copy link
Contributor

pherbel commented Feb 2, 2015

2nd topic of the "Ideas for Contributions" list
https://github.com/dotnet/orleans/wiki/Ideas-for-Contributions

@pherbel
Copy link
Contributor Author

pherbel commented Feb 2, 2015

I'm checking it how it is possible to update, but It seems the 1.7 version is quite old so it is not so easy

@BenjaminGibbs
Copy link
Contributor

When the storage library moved to version 2.0 the storage team implemented a new way of using the library. However they kept an implementation that worked with legacy code in Microsoft.WindowsAzure.Storage.Table.DataServices, a few breaking changes where introduced but on the whole shouldn't be too hard to upgrade the library.

However it might be worth considering updating the whole Orleans codebase to use the newer aspects of the Storage Library, for example the Table Service Layer via TableEntity offers significant performance and latency improvements over the 1.7 lib.

@gabikliot
Copy link
Contributor

We agree. This is our plan indeed. We even had a change for the APIs prepared but the bigger challenge was the change in errors being returned and their semantics. Need to make sure all errors are handled correctly, in all cases.

@veikkoeeva
Copy link
Contributor

A bit off-topic for this thread, why is Microsoft.WindowsAzure.ConfigurationManager set to version 2.0.0.0 in OrleansAzureUtils?

For what it's worth with my rather small piece of software, I've installed the MSI package and referenced the binaries therein and use the latest Nuget packages for Microsoft.WindowsAzure.Configuration, Microsoft.WindowsAzure.Storage etc. with binding redirects and everything looks like working without a hitch (this is how I've ran Orleans starting from the first publicly available bits).

@gabikliot
Copy link
Contributor

Sounds good. Would you like to contribute it as a pull request?

@veikkoeeva
Copy link
Contributor

@gabikliot Heh, I merely meant that I just added the newest libraries, redirects and went off with a fairly small application without much consideration if it really works. At least it works on a emulator and Azure roles as far as I can see. :)

I can give the actual code an actual stab later today. I took a look at AzureTableManager and some other places quickly and converted almost all of the code in AzureTableManager (it felt rather straightforward, though I'm not sure about some of the "update rules" on entities). I apologise beforehand already that I may not be able to finish this with the quality I'd be satisfied and try to dish out further testing, quality inspection etc. to other people. Perhaps someone finds the "raw conversion" useful.

I read the following sources and I didn't spot anything breaking error codes at leat:

In addition, I think there are plenty of opportunities to clean up the code and probably make it faster too in the process. One thing to look at is probably also how the Nuget dependencies will be handled. Anyway, I'll try to jot down some notes and make a PR for discussion later today.

@pherbel
Copy link
Contributor Author

pherbel commented Feb 8, 2015

I've dived deeper into the Orleans Azure storage implementation and I see that the storage upgrade could be easy if we are staying on the DataServices version (Microsoft.WindowsAzure.Storage.Table.DataServices namespace after the 2.0). Actually it has some improvements, but the whole stuff is legacy and obsolete. It's okay, but I can't see any benefit.

Essentially, more beneficial if we will use the new implementation but it is significant change in the code and interface.
I can see this benefits:

Currently I'm working on the new version, but it is hard to do. But it's worth it because it will be much faster.
@gabikliot Is there any internal integration test to validate the storage change? I mean after the change We should see the error handling and the retry logic are still good.

PR coming soon

@veikkoeeva
Copy link
Contributor

@pherbel I updated to the latest Nuget Storage and Configuration packages and refactored the code to use the newer mechanisms. The code compiles and the tests pass, but I don't think it's ready until someone takes it to to an actual spin and checks the code (see my PR notes for more details), but it should give a good direction where to go. If you want to take the code and meddle with it, go ahead. I think the earliest I'm onto this is the next weekend (if I have time).

You are right that using the newer stuff the code would be both faster and (a lot) mor succinct. I'll put some more notes in the next few days. It would seem prudent to plan that refactoring as a separate item.

As a tangential note, is there nothing that stops someone upgrading to .NET 4.5.1 (preferably after this storage update)?

@gabikliot
Copy link
Contributor

We are all for moving to the latest version, due to all the reasons you mentioned above. The change is indeed far from trivial, but the good thing is it can almost entirely be kept local to one file: AzureTableManager, which abstracts away the Azure Table APIs.

If we have other ideas, beyond moving AzureTableManager to 2.5, how to make things around storage better, lets keep them as separate issues and separate PRs. It will make handling this easier.

@sergeybykov
Copy link
Contributor

I submitted #158 that should complete the initial migration. More refactoring can be done after that.

sergeybykov pushed a commit to sergeybykov/orleans that referenced this issue Feb 24, 2015
…nager 2.0.3 Nuget packages updated with associated code changes.

Fixes dotnet#79.
This was referenced Feb 24, 2015
sergeybykov pushed a commit to sergeybykov/orleans that referenced this issue Feb 24, 2015
…nager 2.0.3 Nuget packages updated with associated code changes.

Fixes dotnet#79.
@gabikliot
Copy link
Contributor

Fixed via #163.

@ReubenBond
Copy link
Member

Awesome! Should we push a NuGet release (or perhaps a prerelease)?

@gabikliot
Copy link
Contributor

We still want to do another pass, of cleanup, taking some of the additional feedback from @veikkoeeva and others about small perf. improvements, etc... This should not take long now. The biggest challenge for this PR was updating our internal test infrastructure to the new storage version as well. Now that we are done with it all any new fixes should be much easier.

@gabikliot
Copy link
Contributor

Followed up with #168, which is now merged.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants