Skip to content

Conversation

@veikkoeeva
Copy link
Contributor

This resolves #79 when the converted code works as intended.

With this I sought to see approximately how much effort it would take to stab the storage system and what issues there would be. There aren’t many storage related tests and I thought that creating them would take too much effort for what I aimed at. I am not sure if this would be the best approach forward. One could reason testing (and fixing) these new binaries could be done observing by experience if they behave well enough. That is, there are other means besides a test harness to be assured of functionality, e.g. logs, alerts, experience of running a sufficiently busy system. If nothing else, this refactoring attempt presents some way to transform the code to quite close the needed functionality.

The resulting code is not good nor is it even correct in all parts. This uses the new binaries, but there needs to be cleanup especially in AzureTableDataManager storage methods. Even after that, it feels it’s somewhat a throw-away phase. Albeit this might be the case, I feel it would be an important step when assessing the conformance of this new version with the previous one. Especially this version would allow using newer libraries with the Orleans Nuget packages. In the meantime there could be incremental performance updates and even a new storage system implementation. There’s a blog post Windows Azure Storage Client Library 2.0 Tables Deep Dive which explains the newer constructs quite well.

Some spots that look potential bugs and cases for improvement
  • DeleteTableEntriesAsync(IReadOnlyCollection<Tuple<T, string>> list)
    doesn’t check the list length. The batches are limited to 100 elements by Azure runtime. Even better would be take an arbitrary length list and chunk it to appropriately sized batches.
  • It’s counterpart BulkInsertTableEntries(IReadOnlyCollection data)
    does have a check, but it could also do chunking.
  • ReadTableEntriesAndEtagsAsync(Expression<Func<T, bool>> predicate)
    look like do not need complicated custom error handling anymore.
  • Currently ClouldTableClient is passed as a parameter to a lot of function. Perhaps better would be to pass CloudTable, as usually the immediate call is var tableReference = tableClient.GetTableReference(TableName); to obtain a reference and then use it.
  • Perhaps functionality to explicitly resolve entity type and do reflection isn’t needed anymore with refactoring the storage functionality.
  • In ReminderTableEntry the ETag is shadowed.
  • This refactoring isn't ready. There are todos, which should be turned into issues, commented code that ought to be removed and, as mentioned, issues with how to update, insert, upsert, merge entities. This could be minor issues if someone points out the desired behaviour in terms of the new library (or straight HTTP verbs).
How to proceed
  • I don't mind other people taking this code to make progress or adding to this here (I'm not sure how to do that, but probably doable).
  • Can someone provide tests to provide the correct functinality or run the refactored code to attain a reasonable idea if it works correctly? If not, then perhaps one path forwards would be to capture data from a present system, create test material and check the results of the new system against this material. This probably needs to be done soon after this update anyway (but perhaps this update may alter the way entities are stored, which is desirably differently than currently).
A bold aim

I think the bolder aim could be to provide a stream to storage, or two streams. One stream could save straight to storage, the other could batch messages and save (adaptively on load?) based either on some time interval or elements in the queue. This stream perhaps could have a configurable failover so that when, say, Azure Table Storage (ATS) goes offline, a secondary system would be attempted (a SQL Server, or another ATS storage (via connectionstring)). This system could be perhaps provide library components to a common pubsub functionality. This kind of batching doesn’t look like too complicated from an Rx perspective. As an example: http://blog.petegoo.com/2013/01/20/reactive-extensions-in-message-processing/ (though delay sensitive stream would be more complicated).

These streams could, naturally, provide a place to intercept the messages.

@veikkoeeva veikkoeeva changed the title pdated Microsoft.WindowsAzure.Storage and Microsoft.WindowsAzure.ConfigurationManager to the latest Nuget packages and code converted accordingly Updated Microsoft.WindowsAzure.Storage and Microsoft.WindowsAzure.ConfigurationManager to the latest Nuget packages and code converted accordingly Feb 8, 2015
@gabikliot
Copy link
Contributor

Thank you @veikkoeeva
Can I ask you please to update the PR so it does not modify so many files. It appear that you have renamed the word "table" to "tableReference" in the comments in a lot of files and it makes reviewing it hard.
Once this is done the change should probably not span more than 5 files.

The general direction of keeping all changes inside AzureTableDataManager and keeping all the other code unmodified is right. That was the intent exactly.

@veikkoeeva
Copy link
Contributor Author

@gabikliot Hmm... I'm not sure how to change only a small number of files as when I upgrage the storage library the solution doesn't compile anymore without touching quite many files.

I believe one nice outcome is that there are some pieces of code it nothing else. Even that took some time to think about, at least for me. :) With some luck, I get to the hoods, so to speak, next weekend. Though I don't mind if someone wants to just copy the piece I changed over to another branch with perhaps more moderate changes.

I started by upgrading the storage library only in Orleans, but after a bit refactoring Visual Studio started crashing or the Intellisense stopped working. I believe one of the key reason to this was the MSBuild assembly references inside Orleans.csproj. Then the problems spread to other projects that linked Orleans, which is quite many (and I needed to upgrade the storage dll there too).

As written in #79, this is more in the class of a good stab and a serious try, but not carried out with utmost rigor (e.g. I didn't capture data from running system, write tests and refactor after that) and hence more action is needed. There are problems and similarly changes for optimizations as alluded already, but I need to write about them tomorrow, in about 20 hours.

Of, the most interesting case is AzureTableDataManager. The various "update with some rules" weren't easy to interpret in terms of HTTP verbers (GET, POST, PUT and PATCH/MERGE) which, I believe, is the sematics table storage uses. So, I just tried to replicate the existing functionality with the new libraries even if it did feel it could be simplified to, say, MERGE.

<edit:
I renamed the variables akin to TableServiceContext svc = tableOperationsClient.GetDataServiceContext(); to var tableReference = tableOperationsClient.GetTableReference(TableName); as there isn't ServiceContext anymore in the new library. Though it seems to be, for a reason unknown to me, that a lot of comments and logging strings have changed too. I try to revert those changes (if someone doens't get there first). That shouldn't have happened.

@gabikliot
Copy link
Contributor

I mostly meant not doing the renaming of the word "table" to "tableReference" in the comments in a lot of files, which makes reviewing very hard. If you undo all these, the number of other changed files is manageable.

@veikkoeeva
Copy link
Contributor Author

I mostly meant not doing the renaming of the word "table" to "tableReference" in the comments in a lot of files, which makes reviewing very hard. If you undo all these, the number of other changed files is manageable.

Yeah, I think I got that. Mm, I need to put the kids to sleep now, but I'll see what I manage to do this evening. As you point out, that renaming doesn't make sense.

@gabikliot
Copy link
Contributor

No rush, this can wait. Thank you for your help!

@veikkoeeva
Copy link
Contributor Author

@gabikliot ... And some updates later. Yes, after every commit I thought I had it fixed and after pushing I thought that okay, let's not squash it anymore. Shouldn't work at midgnight, ugh.

Oh, there are some comments and TODOs to grab attention. They ought to be cleaned up and possibly turned into issues. Hmm, and a few spurious empty space changes, luckily only few. Should check my VS settings... Anyhow, I'm off to bed now. Do reading people. :)

@gabikliot
Copy link
Contributor

Thank you. This is much more compact now.

@veikkoeeva
Copy link
Contributor Author

@gabikliot and others, I updated the initial comment. I'll try to be more of a laborer here as I don't currently have that much time to think this. So, any ideas are welcome and I'll try to provide assistance wherever I can. If this feels like a good path forward, I can fix issues as they emerge and naturally welcome others too to do the same.

@veikkoeeva
Copy link
Contributor Author

@gabikliot, @jthelin-microsoft I was wondering if I should add a storage tests to the project. Say, something like Orleans.StorageTests that would then kick up storage (and compute) emulator and possible to other storage in the future, such as LocalDb. Would that be a good idea? To this PR so as to facilitate testing?

I was thinking a bit ahead today that perhaps in the future there's a need for SQL Engine storage too. It's not that bad at logging or queuing, for instance, and can become cheaper too in Azure. Doing it with SSDT is handy. I set up one such system at work, it build the database, deploys, creates dacpacs, runs integration tests (through two IISs to db, which get reset appropriately) etc. and all that relying on a basic VS installation with LocalDb.

@gabikliot
Copy link
Contributor

I am porting a bunch of Azure Table storage tests that we have internally and will submit a PR soon. So you better wait until you see what is already there. If you find a need to add more above it, you are welcome.

We already support SQL storage for all system tables: https://orleans.codeplex.com/wikipage?title=What%27s%20new%20in%20Orleans%3f&referringTitle=Orleans%20Documentation

and we have a bunch of tests for that as well.

@jthelin
Copy link
Contributor

jthelin commented Feb 12, 2015

PR #114 contain the Azure tests that @gabikliot mentioned, and those tests are now merged into master.

@veikkoeeva
Copy link
Contributor Author

Thanks. I updated to the latest storage and configuration packages, fixed the new code and pushed. I did git pull --rebase upstream master, but somehow this looks a bit weird in GitHub now, but if this ends up to master, it can be fixed and if only some pieces get applied in somehow, it doesn't matter. O_o

In any event, the following tests will fail now (I didn't investigate further):

  • AzureMembership_ReadAll_1
  • AzureMembership_ReadRow_1
  • SiloAddress_ToFrom_RowKey

@veikkoeeva
Copy link
Contributor Author

Rebased on the latest master. I'm in a bit of bind at leas this weekend as to what comes to investigating the tests and fixing them and fixing the ported code, but here are results of the three failed tests:

Test Name: AzureMembership_ReadAll_1
Test FullName: UnitTests.StorageTests.AzureMembershipTableTests.AzureMembership_ReadAll_1
Test Source: c:\projektit\Orleans\src\TesterInternal\StorageTests\AzureMembershipTableTests.cs : line 172
Test Outcome: Failed
Test Duration: 0:00:00.1833918

Result Message: Assert.AreNotEqual failed. Expected any value except:<W/"datetime'2015-02-14T15%3A08%3A32.103Z'">. Actual:<W/"datetime'2015-02-14T15%3A08%3A32.103Z'">. New VersionEtag differetnfrom last

Test Name: AzureMembership_ReadRow_1
Test FullName: UnitTests.StorageTests.AzureMembershipTableTests.AzureMembership_ReadRow_1
Test Source: c:\projektit\Orleans\src\TesterInternal\StorageTests\AzureMembershipTableTests.cs : line 132
Test Outcome: Failed
Test Duration: 0:00:00.2140295

Result Message: Assert.AreNotEqual failed. Expected any value except:<W/"datetime'2015-02-14T15%3A08%3A31.88Z'">. Actual:<W/"datetime'2015-02-14T15%3A08%3A31.88Z'">. New VersionEtag differetnfrom last

Test Name: SiloAddress_ToFrom_RowKey
Test FullName: UnitTests.StorageTests.SiloInstanceTableManagerTests.SiloAddress_ToFrom_RowKey
Test Source: c:\projektit\Orleans\src\TesterInternal\StorageTests\SiloInstanceTableManagerTests.cs : line 239
Test Outcome: Failed
Test Duration: 0:00:00.130572

@veikkoeeva
Copy link
Contributor Author

Rebased on the latest master. All tests pass now. @gabikliot Could someone take a look if there's something that needs still to be done? I haven't myself installed the SDK generated from this code and it may take a while, as everything lately. :)

Of course, the issues regarding code refactoring to better utilize the new SDK still needs to be carried out.

@gabikliot
Copy link
Contributor

Thank you @veikkoeeva! We will take a look. So the code passed all the unit tests, as is, with no changes? Nice! Did you run against the storage emulator or real Azure storage?

What "issues regarding code refactoring to better utilize the new SDK " are you talking about?
I think one thing is pretty sure: we want to stay with AzureTableDataManager and AzureQueueDataManager abstractions. All Azure storage specific APis should be hidden within those 2. We don't want to expose internal Azure storage APis outside those, since in the places where we use those we also use SQL system provider and may use more in the future, so the next level above that uses AzureTableDataManager is generic and cannot know anything about Azure storage.

Inside AzureTableDataManager and AzureQueueDataManager we can do what ever we want. I thought that is what you did now.

@veikkoeeva
Copy link
Contributor Author

@gabikliot The tests are unmodified, I ran them on Azure Emulator. I have a perhaps an unjustified feeling the tests won't cover all the cases.

As for refactoring, these new libraries should bring a more performant storage layer in general, but there are things one could do, taking cues from Windows Azure Storage Client Library 2.0 Tables Deep Dive and especially from Jeffrey Richter's (ed. of Wintellect fame) Guide to Working with Azure Storage Tables:

  • Now the entities are refactored to use TableEntity, which uses reflection. DynamicTableEntity would probably better and it looks like it's essential in case one wants to employ storage data type updates without incurring downtime. The old code looks like uses reflection too and as that wouldn't be just updating the libraries and strictly associated code, I left it for some other PR that does further refactoring and optimizations.

  • Pieces of such as

    List<T> results = await AsyncExecutorWithRetries.ExecuteWithRetries(
                    counter => executeQueryHandleContinuations(),
                    AzureTableDefaultPolicies.MaxTableOperationRetries,
                    (exc, counter) => AzureStorageUtils.AnalyzeReadException(exc.GetBaseException(), counter, TableName, Logger),
                    AzureTableDefaultPolicies.TableOperationTimeout,
                    backoff);
    

    looke like unnecessary with the new library. It offers linear retry and exponential backoff operators and let's one to write custom ones too behind the IRetryPolicy interface.

  • Chunking of updates and inserts instead of just checking there should be at most 100 entities in a batch and erroring out.

  • Maybe some other PR fixes, say, some typos I spotted. I tried to be careful for not touching too many places, as this got quite a large batch already.

Then possible bugs and erros:

  • In the two *ConditionallyAsync functions I wrote to take the Etag from the 1-2 operations like return opResults[0].Etag; but this may not be correct. Which operation Etag should be returned and does it matter? I took a look again the non-refactored code and I think it was clear which ETag was needed. I added a comment to code and I think it is now as it should. I took the liberty to squash the fix to my previous commit, rebased on the latest master and forced the updates to this branch (N.B. if anyone has previous history stored).
  • DeleteTableEntriesAsync doesn't check if the batch is over 100 entities. Maybe it should be checked. I added a check to see if the list is null or empty, as AzureMembershipTableTests Cleanup method crashed to an exception where the Storage library told the batch could not be empty when I ran some single tests and not all of them.

…nager 2.0.3 Nuget packages updated with associated code changes.

Fixes #79.
@gabikliot
Copy link
Contributor

Thank you @veikkoeeva. We will take a look at those.

@veikkoeeva
Copy link
Contributor Author

@gabikliot Just noticed that UpsertTableEntryAsync doesn't do exponential backoff of retries as the in the previous versions. I think it needs to be fixed. Can you spot anything else?

@gabikliot
Copy link
Contributor

@veikkoeeva Can you please update the pull request to the latest master version? I just pulled it now and had a bunch of conflicts. Thank you.

@gabikliot
Copy link
Contributor

@veikkoeeva I have a question. How would we like to handle the process of accepting this PR? I will most certainly have a bunch of comments, will need to change/fix a bunch of small things. One way is for me to pull this PR, work on it, modify all I need, test it, and then submit a new PR. But that would mean the commits would be named with my name. But you did most of the work, so it’s fair it comes under your name.
Alternatively, I can do all the fixes, submit a PR which you will pull locally to your repo and resubmit essentially the same PR under your name. Would that work?
The third option I see is for me to write all the comments to you and keep doing all the change via code review. I think I prefer not taking this last route, since it will be very time consuming, will take a lot of iterations, ... I prefer to get there quicker. Is there another way?
What would be your preference?

@gabikliot gabikliot self-assigned this Feb 16, 2015
@gabikliot
Copy link
Contributor

So I merged al the conflicts and have it building now.
We also figured out the right procedure w.r.t. commits. I pulled your requests to my local branch, merged conflicts and committed to my local branch. This now has all the history: both your commits as well as my later commits. So now I can safely keep changing it and at the end when I submit this PR it will have all your commits in it.
I got it from now on. Thanks!

@gabikliot
Copy link
Contributor

Hi @veikkoeeva. I am reviewing the code now. I spotted one key place: UpdateTableEntryConditionallyAsync does not do anything with the provided eTag. It just does Replace or InsertOrReplace. The key point about this function was that the update should be conditional, based on the eTag.
Am I correct in saying that it does not handle the eTag correctly now, and needs to be updated to do so? I will handle the code update, I just want to make sure I did not misinterpret the code.

It is also not clear how to do that in the new APIs. In the old API we did:
svc.AttachTo(TableName, data, dataEtag);
svc.UpdateObject(data);

@veikkoeeva
Copy link
Contributor Author

@gabikliot You are right, difficult these "refactor during the night" sessions. :) Hmm, it should be assigned to data.ETag if it needs to be used. On the other hand, if it will be always the one in data there is no need.

As you might noticed, I refactored out already some code that was used internally to just retrieve the entity ETags after operations. The new storage library assigns the ETag automatically to entities after storage operations, based on HTTP header values in replies. You might find the article Storage Client Library 2.0 – Migrating Table Storage Code as a handy migration guide between the old and new style of code. Depending on operation, the ETag value determines what happens (notice the * and null as special values), one place to look at the semantics is at MSDN Operations on Entities (Udate in this link, see the topics panel for others).

One of the troubles I had was that I have only used the newer library and in general think this in terms of the "HTTP semantics" and I wasn't sure how the old library functions and what exactly the current/old code should be doing.

By the way, one additional, low-hanging fruit type of optimization could be PayloadFormat = TablePayloadFormat.JsonNoMetadata. I'm on GMT +2 and I suppose you've gone to asleep now. I'm not sure if you can update my fork of Orleans directly or should I flip some switches to give rights, but I don't mind you or someone else doing additional fixing. If there's something I can do, I'll be checking this a few times during the day, but real work starts probably, hmm, in about 14 hours.

@ReubenBond
Copy link
Member

👍 @veikkoeeva, Very nice work! Is there anything your is PR is missing, or is it good to go?

@gabikliot
Copy link
Contributor

@ReubenBond , are you asking about Azure storage move to 4.2 PR? I am currently integrating this PR. There are a couple of small fixes that needs to be done (around eTags), so the PR as is now won't work correctly.

@gabikliot
Copy link
Contributor

I recommend waiting a day or 2. I am on it right now.

@ReubenBond
Copy link
Member

Thank you, @gabikliot! I'll work around it for now.

@veikkoeeva
Copy link
Contributor Author

@ReubenBond Yep, @gabikliot is on the rest of stuff currently. This started as a hack that got serious, so I left @gabikliot some work to fix. :) @gabikliot Don't forget my comment on exponential backoff.

If there's anything I can do, push the newest code somewhere and give a notice and I'll get on it as soon as possible.

@ReubenBond
Copy link
Member

Thank you both :) your efforts are very much appreciated!

-----Original Message-----
From: "Veikko Eeva" [email protected]
Sent: ‎18/‎02/‎2015 15:32
To: "dotnet/orleans" [email protected]
Cc: "Reuben Bond" [email protected]
Subject: Re: [orleans] Updated Microsoft.WindowsAzure.Storage andMicrosoft.WindowsAzure.ConfigurationManager to the latest Nuget packages andcode converted accordingly (#100)

@ReubenBond Yep, @gabikliot is on the rest of stuff currently. This started as a hack that got serious, so I left @gabikliot some work to fix. @gabikliot Don't forget my comment on exponential backoff.
If there's anything I can do, push the newest code somewhere and give a notice and I'll get on it as soon as possible.

Reply to this email directly or view it on GitHub.

@pherbel
Copy link
Contributor

pherbel commented Feb 18, 2015

@veikkoeeva Very nice work! Actually I also worked on this but I had no time finish yet.
So I just write to here my comments about difference my version and yours.

Would be more readable if you use the CloudTable Async methods
like this:
TableOperation insertOperation = TableOperation.Insert(data);
TableResult result = await tableReference.ExecuteAsync(insertOperation);
return result.Etag;

instead of this:
var opResult = await Task.Factory.FromAsync(
tableReference.BeginExecute,
tableReference.EndExecute,
TableOperation.Insert(data),
null);

I think we should consider to catch a StorageException on each try-catch block not just the generic Exception.

@gabikliot Are you planning to change the interfaces around the ETag? I'm just wondering because the Tuple<T, string> and the separated tableVersionEtag don't need any more.

I think ReadSingleTableEntryAsync and DeleteTableEntryAsync should use directly the storage client and not the batch version because of performance reason (haven't tried)

Instead of
Expression<Func<T, bool>> query = instance => instance.PartitionKey == partitionKey;

We should use this below because of performance reason
TableQuery.GenerateFilterCondition("PartitionKey", QueryComparisons.Equal, partitionKey)
I assume it will filter on server side.

That's it for now. I will go through on it again but I hope this was helpful.

Thanks again @gabikliot and @veikkoeeva

@gabikliot
Copy link
Contributor

I am working on this PR right now.
There are a number of things that needs to be done:

  1. proper handling of eTags
  2. proper handling of exceptions. The exceptions changed since the old version, it is all StorageException now, so the old code that analyses exceptions does not work correctly. As a result any code that relies on correct handling of eTags is not working right now.

My goal is to get a correct, fully functional implementation first and accept the PR at this point.
Then as a 2nd step look at improving the APIs, Dynamic Entity, non-batch calls, and other things, like you both suggested.

One thing:
you wrote: " I'm just wondering because the Tuple and the separated tableVersionEtag don't need any more."
Both are needed. This is due to a special usage of batch transitions in our membership protocol. We update 2 rows, each with its own eTag.
More details here:
https://github.com/dotnet/orleans/wiki/Cluster-Management

Anyway, this is not really important at this point. First I want to get a fully correct, functionally equivalent implementation. Only after that look at improvements.

@veikkoeeva
Copy link
Contributor Author

TableOperation insertOperation = TableOperation.Insert(data);
TableResult result = await tableReference.ExecuteAsync(insertOperation);
return result.Etag;

instead of this:

var opResult = await Task.Factory.FromAsync(
tableReference.BeginExecute,
tableReference.EndExecute,
TableOperation.Insert(data),
null);

It would be cleaner, but I've understood using the latter is marginally more performant.

@gabikliot

One thing:
you wrote: " I'm just wondering because the Tuple and the separated tableVersionEtag don't need any more."
Both are needed. This is due to a special usage of batch transitions in our membership protocol. We update 2 rows, each with its own eTag.
More details here:
https://github.com/dotnet/orleans/wiki/Cluster-Management

I think @pherbel meant that the entities themselves have an ETag that gets updated along with storage operations, so why pass it as a separate parameter and not use the on in the entity. This is something I didn't do (I remember) in the ConditionallyAsync flavour of functions and left the parameter just unused and up for removal in a later refactoring. It looked like the entity already had the same ETag. Though if it is needed, it should be assigned to the appropriate entity ETag entity (and later, after refactoring, already after the function call).

Otherwise I think the exception handling was a dark spot. I could see that the namespaces got updated, but it would be difficult and laboursome to verify without extensive tests if they are all as they were before or if not, how they should work. That's where @gabikliot steps in, luck with us. 👍

As a side note, I thought about updating the test project so that it starts the emulator automatically. It would feel like a nice feature especially on a CI machine. Perhaps also something that downloads and installs the correct version if needed.

@pherbel
Copy link
Contributor

pherbel commented Feb 19, 2015

@gabikliot

My goal is to get a correct, fully functional implementation first and accept the PR at this point.
Then as a 2nd step look at improving the APIs, Dynamic Entity, non-batch calls, and other things, like you both suggested.

Makes sense

About the eTag @veikkoeeva exactly wrote down how I meant
BTW There is a very useful post about the storage concurrency. managing concurrency
There is a table in this post which describes when you should deal with the eTag these are only the Update, Delete, Merge method

@veikkoeeva

It would be cleaner, but I've understood using the latter is marginally more performant.

I see

Another improvements would be a custom retry policy implementation.

@gabikliot
Copy link
Contributor

Hi @veikkoeeva @ReubenBond @pherbel
We have got a Release Candidate for that fix. It includes all the fixes by @veikkoeeva, plus Exception handling, plus eTags, plus a number of other smaller fixes. It is set to Azure Storage version 4.2.
Here:
https://github.com/gabikliot/orleans/tree/AzureStorageUpdate-CLEAN

So far it passes all our tests, but we are still running a bit more tests on it, to make sure.
If you have time to try it out and tell us if it works OK for you, that would be great!
Thank you.

@ReubenBond
Copy link
Member

Thank you, @gabikliot! Trying the bits, I'm getting a "Azure storage connection string cannot be blank" which I wasn't getting before. It looks like it's pulling the connection string from Azure configuration instead of the silo config file.

I'm not running in Azure, but I'm using an Azure system store.

@gabikliot
Copy link
Contributor

Reuben, nothing changed around that. What are you running? Azure web sample? I think it is supposed to pull it from azure config in azure web sample and in any real azure deployment. I think azure host expects it there. Anyway, I think this is unrelated. If you set the connection string correctly, does it work for you ok in all your scenarios?

@ReubenBond
Copy link
Member

I think I know what's up: the NuGet packages were copying default config which was overwriting my own config. I've remedied that and am testing again.

@veikkoeeva
Copy link
Contributor Author

It's about six o'clock in the morning here, I'll try try in about fifteen hours if you haven't merge this by then. I'm excited!

@ReubenBond
Copy link
Member

It looks like my current problems are caused by differing versions of the Azure storage libs.

I'm seeing an exception on silo init: Provider of type Orleans.Storage.AzureTableStorage name AzureTable was not loaded.

I also see this in the log:

OrleansSilo.exe Information: 0 : [2015-02-20 04:19:47.529 GMT     6 INFO    100000  AssemblyLoader.Silo 192.168.0.3:31002]  User assembly ignored: C:\WFDevCluster\Data\Node.2\Fabric\work\Applications\Application_App9\OrleansSilo.Code.1.1.384.0\orleansproviders.dll
* An assembly dependency [Microsoft.WindowsAzure.Storage, Version=4.2.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, Could not load file or assembly 'Microsoft.WindowsAzure.Storage, Version=4.2.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)] could not be loaded: 0 
OrleansSilo.exe Error: 0 : [2015-02-20 04:14:57.228 GMT     8   ERROR   103108  ProviderLoader/IStorageProvider 192.168.0.3:31002]  !!!!!!!!!! Provider of type Orleans.Storage.AzureTableStorage name AzureTable was not loaded.   
A first chance exception of type 'Orleans.Runtime.OrleansException' occurred in Orleans.dll
A first chance exception of type 'System.AggregateException' occurred in mscorlib.dll
OrleansSilo.exe Information: 0 : [2015-02-20 04:14:57.275 GMT    12 INFO    100508  Catalog 192.168.0.3:31002]  After collection#1: memory=35MB, #activations=0, collected 0 activations, collector=<#Activations=0, #Buckets=0, buckets=[]>, collection time=00:00:00.0516011. 
OrleansSilo.exe Error: 0 : [2015-02-20 04:14:57.275 GMT     5   ERROR   100439  Silo    192.168.0.3:31002]  !!!!!!!!!! Exception during Silo.Start  
Exc level 0: System.AggregateException: One or more errors occurred.
   at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
   at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
   at System.Threading.Tasks.Task.Wait(TimeSpan timeout)
   at Orleans.OrleansTaskExtentions.WaitWithThrow(Task task, TimeSpan timeout)
   at Orleans.Runtime.Silo.DoStart()
   at Orleans.Runtime.Silo.Start()
Exc level 1: Orleans.Runtime.OrleansException: Provider of type Orleans.Storage.AzureTableStorage name AzureTable was not loaded.
   at Orleans.Providers.ProviderLoader`1.ValidateProviders()
   at Orleans.Runtime.Storage.StorageProviderManager.LoadStorageProviders(IDictionary`2 configs)
   at Orleans.Runtime.Scheduler.SchedulerExtensions.<>c__DisplayClassa.<<QueueTask>b__8>d__c.MoveNext()
A first chance exception of type 'System.AggregateException' occurred in OrleansRuntime.dll

Is there some way that we can also support newer versions of the libs?

@veikkoeeva
Copy link
Contributor Author

@ReubenBond Hmm, shouldn't there be the 4.3.0 version of the libraries in the Orleans solution? As a tangential, I wonder if the next Orleans release should include a Update-All on packages and if so, should it be tested the same time as this just to make sure? Even if it's not strictly the business of this PR. A refrence to issue #125

@ReubenBond
Copy link
Member

Maybe we can update all the way to 4.3.0?

@gabikliot
Copy link
Contributor

@ReubenBond We will think about it. We have other big customers who are still on 4.2, so we need to find a solution that will satisfy everyone.
Is there any reason you prefer 4.3 over 4.2? Were there any important bug fixes or new features, or is it just the latest one you use, but in principle there is no reason that 4.2 would not work for you?

@sergeybykov
Copy link
Contributor

The general thought was to require latest version -1, so that people aren't forced to upgrade to latest right away, but can do so easily if they are ready. Do you see any problem with this approach?

@ReubenBond
Copy link
Member

I wonder why binding redirection doesn't seem to be working - it should be happy with the newer dlls.

@gabikliot I've got 4.3 because I was using Azure storage for other things

@veikkoeeva
Copy link
Contributor Author

@sergeybykov I don't see a problem here as I can update the packages afterwards (the .nuspec seems to define >= 4.2.0.0, which is good). Tangentially this touches upon how to maintain versions. Should the new versions be branched, released to Nuget and then maintained in their respective branches, and when a new version follows (perhaps having only updated libraries), branched again, released to Nuget and maintained in their respective branches. That way one can pick and choose the versions. I don't see this as an issue as long bigger updates doesn't take too long. Then again, perhaps less work is have the releases tagged and maintain a master branch that has all the fixes, updates etc.

When can we expect this to turn inti Nuget?

To clarify, my Orleans usage is quite limited currently and I strive to update as soon as there are updates and fix problems as they emerge.

@veikkoeeva
Copy link
Contributor Author

It would look reasonable to close this PR (at some point) and merge the branch from @gabikliot. What do you guys think?

@sergeybykov
Copy link
Contributor

@veikkoeeva Do you think we need a branch or just a release tag for such updates? We are in the process of testing @gabikliot's branch while he is off skiing. If there are no issues, I hope to merge it today. Then we'll need to codesign the binaries, rebuild nugets and publish them. I agree - we should close this PR, as your commits are included in @gabikliot's branch.

@veikkoeeva
Copy link
Contributor Author

@sergeybykov I suppose it versioning comes to how the Orleans team would like to support libraries and how to communicate it. For instance, if there is a major, backwards incompatible update to the public API (or dependent components), would the team still want to support previous versions with bugfixes or backporting features. If so, it would look like feasible to branch the release from a master and tag it.

Some induestries or enterprise policies may dictate certain procedures when there are major updates. One central factor in what to use in such projects is support for fix releases as it may be "politically impossible" to update a component when there's a major change (many times someone just looks at the version number).

This remains me that the current update is for 4.2.0 whereas the newest - 1 is 4.2.1. In 4.2.1 there's a fix to a timeout retry bug. It haven't bitten me, but looks like a nice fix. I didn't find release notes for the configuration library. I suppose the important thing here is that it's possible to run an update to the Nuget packages and everything should just work.

Anyhow, I'll close this PR and we'll wait the update from @gabikliot to land master and Nuget. The code is at https://github.com/gabikliot/orleans/tree/AzureStorageUpdate-CLEAN, no PR as of yet. Big thanks to all for your patience!

@veikkoeeva veikkoeeva closed this Feb 20, 2015
@sergeybykov
Copy link
Contributor

@veikkoeeva I agree with you - if there is a major or breaking change in dependencies, it makes sense to branch. For non-breaking minor changes, hopefully, NuGet update will just work.

@gabikliot
Copy link
Contributor

Fixed via #163.

@gabikliot
Copy link
Contributor

Followed up with #168, which is now merged.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate system storage from Azure SDK 1.7 version to 2.5.

6 participants