Orleans: Orleans couchBase Based membership provider (request for review)

[ashkan-saeedi-mazdeh]

I've implemented CouchBase based membershipProvider table. All tests copied from AzureMembershipBase and are passing.

@gabikliot Can you please kindly take a look to find any conceptual issues or ...
https://github.com/ashkan-saeedi-mazdeh/OrleansCouchBaseProviders

As description, extended protocol is not supported since couchBase is not atomic in multi-document operations. I've tried to copy @shayhatsor 's serializable classes and will clean it up based on SQL or AWS implementations.

@shayhatsor if you can take a look it will be good too or anyone else who is an SME on membership providers.

We'll use this in production and I'll implement reminders too and gradually might implement statistics tables and ... as well to not rely on SQLServer at all. If things move forward well, this will be a free viable on premise option for people and can be added to the main repo.

I'll add it to contributions repo after cleanup and making sure it fully works.

[shayhatsor]

All tests copied from AzureMembershipBase

you mean MembershipTableTestsBase ?

and are passing

the tests are for the extended protocol, they pass unchanged ?

[ashkan-saeedi-mazdeh]

@shayhatsor yes the MembershipTableTestsBase and yes I modified the parts regarding TableVersion.

[shayhatsor]

@ashkan-saeedi-mazdeh, FYI @galvesribeiro has just implemented membership using AWS and made the same changes to the tests. I also welcome him to review this PR too since he's now privy on the subject matter.

[ashkan-saeedi-mazdeh]

@shayhatsor yeah Gutemberg is awesome! I knew he is working on it as well, I welcome any help from @galvesribeiro or anyone else of course.

[gabikliot]

I will take a look later this week.

[galvesribeiro]

Great job @ashkan-saeedi-mazdeh I`ll have an eye on it as soon as a finish the SQS Streams on AWS but keep shooting questions on Skype or Gitter when you need. ;)

[shayhatsor]

@ashkan-saeedi-mazdeh, LGTM :+1:
I have some minor suggestions, but they'd be more relevant when you create a PR in the main repo

[galvesribeiro]

Just 2 comments.

1. Why add [JsonObject]/[JsonProperty] on each of the object/properties? Json.Net serialize everything public unless you tag it with [JsonIgnore], it is more clear IMHO.
2. I saw only Json as the serialization for grain state... Do you plan to add binary serialization later?

Overall great work :+1: :smile:

[ashkan-saeedi-mazdeh]

Thanks all @gabikliot @galvesribeiro @shayhatsor

@galvesribeiro yes binary serialization is in my plans. Have to move forward everything in parallel. About JSON attributes, I copied @shayhatsor 's one and didn't clean it up in general yet, I have no logging as well yet and I would like to add a few more error checks and ... as well.

@shayhatsor shoot them anyways since I want to try to go to contribs repo and NuGet in next two weeks and then move inside if anyone else other than me uses this.

I should add reminders for myself and stats as well. no streams for now unless someone else needs it.

Thank you a lot guys for helping me out, I will try to be the same when I know more and people need my help :)

[shayhatsor]

@ashkan-saeedi-mazdeh

I have no logging as well yet and I would like to add a few more error checks and ... as well.

That's what I meant. Also, in the current naming convention, the project should be named OrleansCouchDBUtils. I actually think we should change this to Microsoft.Orleans.Providers.CouchDB, but that's a breaking change that might be a good idea for the next release version.

[galvesribeiro]

That's what I meant. Also, in the current naming convention, the project should be named OrleansCouchDBUtils. I actually think we should change this to Microsoft.Orleans.Providers.CouchDB, but that's a breaking change that might be a good idea for the next release version.

Yes, there is a force pushing a rename/refactory of the whole namespaces and projects for 2.0. Lets see :)

[ashkan-saeedi-mazdeh]

@shayhatsor the guff was worse than that, It's couchBase not CouchDB, Totally different stuff. I should rename that. I meant CouchBaseDB (where DB is Database) but yeah OrleansCouchBaseUtils is the way for now.

[gabikliot]

I looked at CouchBaseStorageProvider.cs.
Mostly worrying is the code in Write. The checks about exists arre not atomic with later insert, so I think this is a bug. I
About Upsert - please look what wrote to @galvesribeiro aout using Upsert. I am a big opponent of Upsert. I think instead people should use TryInsert and TryUpdate. Upsert can be a helper/wrapper method, but it should only come after TryInsert and TryUpdate are there are used in 99% of cases.

About Read - not surewhy you need Exist check before Get. Its simply redundant, since Get must deal with missing data anyway.

Also about a usage of vars:

       var result = await bucket.GetAsync<string>(docID);
        return Tuple.Create<string, string>(result.Value, result.Cas.ToString());

how am I supposed to read that code? What is result? Please don't use vars for method where the return type is not obvious. If it was GetStringAsync, var would be fine. But not here.

[gabikliot]

Membership:
var actuals = bucket.Get(idStrings);//has no async version with batch reads

Well, then you can't use it. Or at least offload to thread pool and join back. But really, if they don't have async version, don't use it. I am saving you tons of future grief.

why don't you use the same base manager in both storage and BR providers? That way yoou only need to make sure you handle concurrency control correctly once.

[shayhatsor]

var actuals = bucket.Get(idStrings);//has no async version with batch reads

I totally missed it, great catch @gabikliot! As he said, you MUST offload this. This makes me suspect that the library isn't truly async. Some vendors (like Oracle`s MySql ADO.NET provider) publish a sync library that returns Task.FromResult() or similar crimes. Please look into the source code of the library to make sure, if not sure, offload everything. My bad, I trusted the tests too much, but when the underlying library is blocking, you might get misleading results in the concurrent tests.

[shayhatsor]

Also, in the IMembershipTable.ReadRow method you check for existence first. Which is redundant, like @gabikliot said about the storage provider code.

[ashkan-saeedi-mazdeh]

@gabikliot thanks, will fix them all as you suggested, On this

Mostly worrying is the code in Write. The checks about exists arre not atomic with later insert, so I think this is a bug. I

The thing is even if someone inserts something after I check exists the insert fails and if someone deletes before my upsert, that will fail too, so how it is a bug? I'll check your comment to @galvesribeiro and his implementation and fix appropriately.

[ashkan-saeedi-mazdeh]

@shayhatsor Thanks again and I'll fix that redundant exist calls, now I remember I wanted to change them and at first did it like that due to laziness and not wanting to look at docs of CouchBase.

The StorageProvier was supposed to be ready other than only having JSON serialization option.

Also @gabikliot @shayhatsor about asynchronous operations, I'll check the source and also will either offload it to thread pool or just use none-batched query which supports async.

[gabikliot]

About write: that means the exists check is redundant.

[sergeybykov]

@ashkan-saeedi-mazdeh it's been 2 years since this was last updated. Is this still relevant?

[ashkan-saeedi-mazdeh]

@sergeybykov no, actually the code now is being maintained mostly by couchbase people and the memebership provider seems to be fine.