Feature/marten

[kunjee17]

I have added marten support. Which will allow to use Postgresql as backend event store. Everything is working and all tests are passing.

Only performance one performance related issue is there. I have added TODO there. It will not hurt right away but required attention for sure.

Let me know what you think.

[Dzoukr]

Great job, @kunjee17! Looking at the TODO, so is it production ready? Once you say so, I'll merge it into master and release new version for Marten. Or we can release it in current state and publish improved version - it is your call. :)

Btw... I think the version of Marten should be v2.0.1, because it is based on API definition of that version (even if it is first release).

[kunjee17]

@Dzoukr There is not issue as of now. The only thing is I am querying all events and then filtering for CorrId. As optional CorrId is not allowed in query. So, again there will be breaking changes that I don't want to do as of now. You can release it and we might get help to solve that issue from people who are using Marten extensively.

I ll update the version and push the code.

[kunjee17]

Done for this one. My opinion you should use it (release it). So, at least we get input where we can improve upon.

If you are Ok, I am thinking of making one InMemory store implementation as well. (Concurrent Dictionary) . It will make things easier to test.

[kunjee17]

There is way I might extend linq support of Marten and allowing it support Option type. http://jasperfx.github.io/marten/documentation/documents/advanced/customizing_linq/

I will look into it.

Or

If possible you can pull these request, because I have changed build.fsx and added fake.sh so it would be easier for me to add other support. I will file a bug for this case, as I don't have experience in meta programming we might get help that way. Meanwhile I ll give PR for InMemoryStore.

[Dzoukr]

Sure, no problem. I'll release this version today. Thanks again, mate!

[kunjee17]

Thanks @Dzoukr , I will file issue for CorrID thingy and also give PR for InMemory store. Thanks for support and work you have done.

[Dzoukr]

No problem @kunjee17, thank you! I have an idea yet and need to know you opinion on this:

Looking at the code it looks like we could call it actually CosmoStore.Postgres right? I mean... there is nothing Marten specific from library consumer's point of view. Or do you rather keep CosmoStore.Marten and make it clear it is based on another layer on the top of Postgres? I'll leave it up to you.

Second thing (I miss that, sorry) is the configuration - I'd rather have something like you use in your test.

type Configuration = {
    Host: string
    Database: string
    Username: string
    Password: string
}

I feel that having IDocumentStore is too abstract.

Please let me know what do you think about it (I can change it by myself to do not give you more work :)) - later this would be breaking change (which nobody likes) so better to resolve now.

[kunjee17]

@Dzoukr Yes. I start with configuration you had. But I did took a little bit code from Marten.Fsharp test suite. And just not to mess with testing I have to change configuration to IDocumentStore. So, It I don't have to change much in testing part of it. If you see I am creating database on the fly and then destroying it. That is all Marten.FSharp work not mine. I just steal it.

I wanted to change but it will require time and discussion you are OK then we can do it later. (Damn we are going to meet for first time and pilling up lot of work for single conference. Sorry for that. )

[Dzoukr]

I saw that part and thinking... we don't have to for (this case) create database. We can start with fact that database must exist before using CosmoStore.Marten (and document that) and make sure we create testing one at the beginning of the tests (so it would be the same code in test suite).

At getEventStore function we will just use something:

sprintf "Host=%s;Username=%s;Password=%s;Database=%s" conf.Host conf.Username conf.Password conf.Database
|> NpgsqlConnectionStringBuilder
|> DocumentStore.For

We can add createDatabase functionality later (in case admin connection string is used) and the API would remain the same?

I mean... it is better to add new functionality with stable API than change the way how this module is configured.

[kunjee17]

@Dzoukr Give me few mins. I ll send you PR for that.

[TheAngryByrd]

@Dzoukr I disagree about using the simple Configuration over the abstract IDocumentStore. IDocumentStore has many overrides and configuration options. Letting the consumer of this library be able to modify those is more beneficial in my opinion.

[Dzoukr]

I got your point, but on the other hand the "shape" of configuration should be the responsibility of this library. If there is something that is on IDocumentStore and is crucial from perspective of this library, it should be added to Configuration.

[kunjee17]

@Dzoukr @TheAngryByrd My personal opinion is to keep it simple for a start. I am pretty sure as more implementation will be added it would be more clear what should be there.

In functional context we should be requiring (event) store, without bothering configuration part. Now, store can be anything having few functions on it. Which are getting used in library. But I guess it will bring one more abstraction (interface) to it.

It is like having web framework where you can swap JSON serialize library by implemented ISerialize interface.

[TheAngryByrd]

@kunjee17 so that's my point exactly, nothing can be configured currently.

NpgsqlConnectionBuilder has about 50 properties but only 4 can currently be set.

Additionally, when creating a IDocumentStore there are many things to be configured such as

Customizing Document Storage Json Serialization Bulk Insert Initial Data Optimistic Concurrency Diagnostics and Instrumentation Tearing Down Document Storage Document Hierarchies

I don't see a point reimplementing these as they're already available and well documented.

I'm ok with having a simple/getting started function, but this should be just a helper and let people get to the more complex building blocks. This simple function should just call the more advanced version.

[Dzoukr]

Well that's the problem of abstraction over abstraction where Marten is layer on the top of Postgres & CosmoStore.Marten sits on the top of Marten. For example Optimistic Concurrency is something that was meant to be handled by CosmoStore, but now the real logic is moved to Marten. Also having IDocumentStore you can make side changes (adding documents directly) out of controlled logic (from CosmoStore perspective).

But I totally agree we should let people to get to more complex stuff and don't want to be stubborn here. 😄

What about having Configuration as union type where you can have Basic (the sane default using just host, username, etc...) and FromIDocumentStore (where you can pass IDocumentStore)?

[TheAngryByrd]

Yeah that's true. Probably need to document CosmoStore isn't using Marten's Event-sourcing features so none of their configurations will apply.

So I'm thinking there might come a time where CosmoStore might need to augment the DocumentStore as well, such as adding Indexes to documents. The way Marten creates a DocumentStore is via a configuration Action

DocumentStore.For(cfg =>
{
  // Mutate StoreOptions 
});

It might be better to have a StoreOptions -> StoreOptions so the user can configure a few things but CosmoStore can add to it if necessary.

[Dzoukr]

I'll leave it up to you guys - you know way better than me. :) If we could have solution fit for easy starters & for advanced IDocumentStore skillers, I'll be happy.

[kunjee17]

I am also not expert. It would be great if @TheAngryByrd can give PR or help me out to set up things.

[TheAngryByrd]

I'll make a PR with the two options I'm thinking about.