Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleaning Snapshots #100

Closed
rehia opened this issue Jan 17, 2017 · 11 comments
Closed

Cleaning Snapshots #100

rehia opened this issue Jan 17, 2017 · 11 comments

Comments

@rehia
Copy link
Contributor

rehia commented Jan 17, 2017

I started a conversation in the issue #94 to talk about overall performance improvements.
I was talking about a feature, where the snapshots are cleaned automatically.
So I started an implementation here https://github.com/rehia/node-eventstore/tree/clean-snapshots, where I implemented the snapshot cleaning feature for in memory and redis stores.
I still need to make some wider tests, but I also wanted to have your opinion on this feature.

I basically added a new option : max snapshots count, which indicates how many snapshots you want to keep at max. So if the value is 5, and you have already 10 snapshots, when you will create the next one, the 6 oldest will be deleted. By default, of course, none is deleted, as it is actually the case.

What do you think about this feature ? Is this the way you think snapshot cleaning can be done ?

Thanks for your feedback

@rehia
Copy link
Contributor Author

rehia commented Jan 17, 2017

At the moment, I have muted other stores tests, because I don't know if I will be able to implement it in every stores.
A little help would be great !
I also don't know how to allow people to contribute to what will become a PR on this project.
Note that, in the base store, snapshots cleaning remain silent (only a warning) if not implemented in the store used by an application

@adrai
Copy link
Contributor

adrai commented Jan 18, 2017

I would do the cleaning in combination with cqrs-domain.
exactly how you have done it... but without waiting for completion (fire and forget)
just catch if there is an error here and log in in debug... => rehia@a945ad2#diff-f641dc482370f1ff504efc63b483ccccR381

@adrai
Copy link
Contributor

adrai commented Jan 18, 2017

It would be nice if you could do the mongodb implementation too (just if you want)...

@rehia
Copy link
Contributor Author

rehia commented Jan 26, 2017

I've done a few things:

  • implementing snaphost cleaning for mongodb and elasticsearch. it seems optimized for elasticsearch, not sure for mongodb...
  • I've just changed a bit when snapshot cleaning is called: even when adding snapshot fails

Let met know if you see other things before submitting the PR

@adrai
Copy link
Contributor

adrai commented Jan 26, 2017

Seems ok,
btw: tingodb has the same api of mongodb ;-)

@rehia
Copy link
Contributor Author

rehia commented Jan 26, 2017

haha 😅
didn't know anything about tingo before 😁
i'll try to do it.
just need to grab the docker image to test.

I also need to make some real world tests on my project, with a huge amount of snapshots, to see if everything is ok.
I'll let you know

@rehia
Copy link
Contributor Author

rehia commented Jan 27, 2017

done !
as long as the tests in real world.
Seems to work pretty good !
Just need a release !

@adrai
Copy link
Contributor

adrai commented Jan 28, 2017

v1.12.0

@rehia
Copy link
Contributor Author

rehia commented Jan 28, 2017

ok thanks @adrai !
i'll take a look at redis indexation to have more efficient scans

@hilkeheremans
Copy link

no time to contribute atm, but just want to leave a quick thank you!

@adrai
Copy link
Contributor

adrai commented Jan 28, 2017

@rehia I had to exclude elasticsearch the search function has some errors (no idea why)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants