AWS DynamoDB Grain State Provider #2007
Conversation
|
I think there is duplication with the other pr. |
|
@gabikliot actually it is not duplicated... I made a mistake... I should wait this PR get merged before create a new branch for the other one. What I did was create a branch locally from this one so I can use the DynamoDB code into the Membership provider :( If you can merge this one I can fix the other one. Thanks |
|
I can review the membership provider. Someone else can review the storage. For me to review the membership provider, please make its pr clean and only include MBR provider code. I already posted some comments there. |
|
@gabikliot yea I'll make it clear but like I said, this PR need to be merged first since I need Lets wait someone merge it here so I can work on the MBR one. Thanks |
|
In case work continues, some short notes I believe are applicable to this too: https://gitter.im/dotnet/orleans?at=57a35c77836d2d021163d9a6. |
| return batch.ExecuteAsync(); | ||
| } | ||
|
|
||
| public Task UpsertItemAsync(T entry) |
There was a problem hiding this comment.
We need a separate Update which always relates to the previous version and updates the new version if previous was not changed
AND
Insert,, which always inserts if there is no prev version or fails otherwise.
Then any higher level retries should be done on top of it.
We can still have upsert for some situations when we don't care, but 95% of the time we do care.
In general, I always say that upsert is very harmfull and most of the time people use it wrongly instead of properly: try insert, if not read and try update. Otherwise, we run into a danger of a blink write.
Specifically, in both storage provider and MBR provider you will need Update, tryInsert and you will not need upsert.
There was a problem hiding this comment.
Adding to @gabikliot note. When developing a storage provider it's important to think of what happens on "duplicate activation". If two duplicate grains attempt a write to the db at the same time, one of them must fail. This is easily achieved by making sure all write operations send the expected version with the data. I think of it as a conditional upsert operation.
|
Guys, dont review the current code. I had changed it and will let you know whenever I get it reopened probably later today. Thanks |
|
Ok, @gabikliot @shayhatsor (and anyone who can) now it is ready for review. Remember that this issue is just the Grain State storage provider and the core DataManager that will be used on other components like MBR, Reminder, etc. As soon as I got feedback I'll update the PR with the changes. I'll wait this PR get completed before I start the refactory on the MBR one. Thanks! |
|
BTW, if anyone want to run the tests, just follow the instructions here that explain how to quickly download and run DynamoDB. No install required. Just unzip and call the Java commandline as explined. |
| /// </summary> | ||
| public class DynamoDBStorage : IStorageProvider | ||
| { | ||
| internal const string TableNameDefaultValue = "OrleansGrainState"; |
There was a problem hiding this comment.
a suggestion, move all the configuration parsing to DynamoDBStorageConfig in order to separate concerns
|
@galvesribeiro, about the tests you've added. How are they different than the tests already found in Orleans? Do they test specific DynamoDB issues ? I suggest making base classes for these tests. In general, the similarity between the Azure Table implementation and the DynamoDB seems like a good candidate for refactoring to base classes. What do you think? |
|
@shayhatsor The tests are most of them the same. Some I added, some I changed. I already talked with @jdom while doing the xunit migration that we should have the black box tests migrated to theories so we test against the abstraction/interface surface with multiple provider implementation. But again, this is indeed a refactory we can do, but I think it is not the focus of this PR. |
|
We will not use theories, as each provider should be able to be developed independent from the rest of the solution, and even if it was, each of them requires a different fixture for set up. We will be moving to a test kit as explained in #2011. The more we add to the same Tester project, the harder it is to port to coreclr by partitioning the work. If this PR doesn't address the test kit refactoring, we can do that later anyway, as a prereq for the migration to coreclr |
| @@ -24,12 +24,12 @@ | |||
| [assembly: InternalsVisibleTo("OrleansRuntime")] | |||
| [assembly: InternalsVisibleTo("OrleansHost")] | |||
| [assembly: InternalsVisibleTo("OrleansAzureUtils")] | |||
| [assembly: InternalsVisibleTo("OrleansAWSUtils")] | |||
There was a problem hiding this comment.
Why is this needed? In newer providers (such as the event hub stream provider) we started doing some work so that it doesn't require any internals, and any external provider could also be developed in the same way as built in ones. Is it possible to avoid this, and if not, let's try to find a way to make it better.
The event hub provider did require some refactoring of the main Orleans assemblies, but in the end it improved them for all stream providers.
There was a problem hiding this comment.
Right now I just had to add that since I'm using ErrorCode enum. I'll create a new enum with all the AWS specific error codes once we finish the review of all the PRs so before we release the code thiss will be out.
|
Ok. Reset on master and made some fixes. The other enhancements suggested here comes with other PR before we start to generate Nugets to the whole OrleansAWSUtils. |
| var value = serviceConfig.Split(new[] { '=' }, StringSplitOptions.RemoveEmptyEntries); | ||
| if (value.Length == 2 && !string.IsNullOrWhiteSpace(value[1])) | ||
| service = value[1]; | ||
| } |
There was a problem hiding this comment.
this block, including the line above it should be refactored as a method. now it's just duplicated 4 times
|
@galvesribeiro, thanks for this great contribution ! |
|
@shayhatsor Thanks for having time to review and for the feedback :) |
Implementation of #2006 which is part of #2005