search

migrating-ravens / RavenMigrations

A small migrations framework to help you manage your RavenDB Instance.
MIT License
53 stars 24 forks source link

Modify Alter.Collection to use Streaming API so it doesn't miss documents if the index changes #17

Closed daniel-chambers closed 9 years ago

daniel-chambers commented 9 years ago

We had a problem where we were deleting and adding documents during an Alter.Collection, and RavenMigrations was successfully migrating the first pageSize of documents but then not migrating any more. This was because the index was getting changed and therefore the paging methodology used was producing changing results, because each paged query to the index is looking at a different version of the index.

I've rewritten it to use RavenDB's Streaming API which is what we should be using if we want to stably enumerate though all documents in the database. The Streaming API will take a snapshot of the index at the time of the query and use that, so we are free to add/remove documents at will without affecting the enumeration. It's also streaming, so we aren't loading the whole DB into memory.

I've also fixed a few bugs that were causing the integration tests to fail.

Let me know if I've missed anything or made any mistakes! :)

dportzline83 commented 9 years ago

Any plans on bringing this PR in? Seems like this is a fairly large bug, if the current code can miss documents.

dportzline83 commented 9 years ago

I'd like to get this pulled in soon. I'd like to do a little research on how the streaming query will handle a stale index first.

dportzline83 commented 9 years ago

It seems reasonable to expect that a migration would throw an exception if the index you're trying to use is stale. I'd like to avoid a scenario where it doesn't throw or warn you in any way, and then you migrate on a stale index without knowing it.

khalidabuhakmeh commented 9 years ago

For streams, it throws an exception unless specified via a parameter

dportzline83 commented 9 years ago

I don't see that in the documentation anywhere. Though, it does look like the out parameter of type QueryHeaderInformation has a property called IsStable, which is possibly supposed to be IsStale?

dportzline83 commented 9 years ago

I'm planning on merging in this PR except for c7c8248. To solve the problem addressed in that commit, I'm liking the solution in #19 where the migrations are filtered to begin with to only those with the attribute and the base class. Any objections to this?

dportzline83 commented 9 years ago

Merged the changes from this PR, except reverting the one commit. But, I rebased it and it got confused. Anyway, looks like I'll have to manually close this PR.

daniel-chambers commented 9 years ago

Looks good. Thanks for pulling that in.