AWS DynamoDB Reminders

[galvesribeiro]

Implementation of #2006 which is part of #2005

[galvesribeiro]

@gabikliot @shayhatsor rebased here. Could you please review it? Thanks

[shayhatsor]

LGTM

[galvesribeiro]

If anyone else doesn't see any problems I can rebase to resolve the conflicts

[gabikliot]

Wait with merging, I haven't reviewed yet.

[sergeybykov]

A general comment about OrleansAWSUtils.

It is great that we are adding direct and easy support for non-Azure clouds. Not only does this expand the "addressable market" and makes the codebase more portable, this also highlights and supports the important tenet of Orleans - that it runs anywhere you need, the way you need.

When we add similar direct support for GCP, we'll cover the "big three" players.

One thing that I think we need to be conscious of and communicate clearly is the current state of the project. Until somebody runs in production with a piece of tech, it's difficult to be sure that all major issues with it have been addressed.

In that light, I'm thinking that we should explicitly set the version of OrleansAWSUtils NuGet package to 1.0.0-beta or something like that, in order to communicate the state of it and set the expectations accordingly without slowing down iterations on the code. Once somebody confirms that they are using it in production, we'll promote it to the general version. I think we should have done the same with the initial version of OrleansSQLUtils, OrleansConsulUtils, etc., but we keep learning as we go.

[galvesribeiro]

Fully agree with @sergeybykov

I also would like to have the providers release versions not locked on Orleans core anymore so starting with AWSUtils (we should definitively rename that for the "new" naming conventions) keep it as 1.0.0-beta and counting.

There are 3 people ready to use it. Lets see their feedback.

[gabikliot]

I fully support the beta version idea.

[gabikliot]

you don't need the read here, right?
You already have the UpsertEntryAsync in the DynamoDBStorage.

[shayhatsor]

In Azure, MSSQL and MYSQL we don't use the incoming etag at all, and the test assumes that. The etag value isn't populated by Orleans on upsert.

[galvesribeiro]

@shayhatsor this is the mehod that fail on that parallel test if we don't read and do that trick, remember?

@gabikliot are u suggesting dont read the latest etag and trust on the one that come in the object? Or just blindly write?

[shayhatsor]

@galvesribeiro, I remember. but there's a bigger issue here, like i mentioned : The etag value isn't populated by Orleans on upsert. so, in AWS, we read before attempting insert/increment. If you remember, i mentioned you can write with a guid, which will remove the need for the read. You didn't want to have different etag implementations for reminders and other stuff, I can't blame you. I think it's OK either way.

[gabikliot]

OK, I recall now what Shay is saying about blind upsert. I will review carefully again in the evening.

[gabikliot]

Ok, I understand the confusion here. The wrong part is that I named the reminderTable.Upsert as upsert. Upsert means: update or insert. What we really need in Reminders table is Unconditionally write. If its new - insert, if not - just overwrite.
In Azure we indeed implemented "Upsert" as InsertOrReplace, which is correct for reminders but wrong for a general correct Upsert. So this: https://github.com/dotnet/orleans/blob/master/src/OrleansAzureUtils/Storage/AzureTableDataManager.cs#L191
is a wrong name for this method. It should be renamed into WriteUncondionaly, or InsertOrReplcae.

In Dynamo DB storage case you actually implemented Upsert corrrecly: its conditional on prev etag. So here in Dynamo DB Reminders table you SHOULD not be calling it. You should just pick a random int for your etag version and write through, overwriting any potential value. No conditional checks.

In the future we need to fix the naming and also remove the picking the random value into the reminders service and not rely on the table implementation to pick it.

@shayhatsor , do you agree?

[galvesribeiro]

@gabikliot ok,understood. Indeed the Azure naming is confusing. In order to fix it, I should use our PutEntryAsync on DynamoDB which Create or Replace an item and just do it unconditionally. Will make the update.

[galvesribeiro]

Done

[gabikliot]

The parallel upsert test should succeed: reminders have different names there. Name is part of row key.

[galvesribeiro]

Sure. Added that and it passed.

[gabikliot]

Re reminders Upsert: #637 (comment)
It's good I explained it there. It would take me a significant effort to recall it from the depth of my mind now.

[gabikliot]

@galvesribeiro , looks good, please squash and rebase.

[galvesribeiro]

@gabikliot @shayhatsor rebased. Thanks for take time to review.

[shayhatsor]

@galvesribeiro, I was going to merge this PR but GitHub says I'm not authorized. Does anyone know what's up? @sergeybykov, @jdom ?

[jdom]

I do not know what's up. Merge button is enabled for me. Let me know if you still can't and whether you want me to push the button. Otherwise we can wait for Sergey to check what's wrong

[shayhatsor]

It's weird, It's still disabled for me. I'll wait to see if it's a temporary GitHub hiccup. No rush at this point, thanks @jdom for the quick reply.

[gabikliot]

Same for me, it says I don't have write access to this repository any more.

[galvesribeiro]

I never had write access here but now I'm seeing same screen you all here with the gray button.

[jdom]

Try now, I think it was my bad, I intended to protect the master branch from force push, but I think I protected it from normal push too. Should be back to normal now

[gabikliot]

Yes, it's good now. @shayhatsor you can merge it.

[galvesribeiro]

Aha! Now I dont see that button again and only the green checks. Thanks