rosedblabs / rosedb

Lightweight, fast and reliable key/value storage engine based on Bitcask.
https://rosedblabs.github.io
Apache License 2.0
4.48k stars 620 forks source link

[Feature] add auto merge background task #298

Closed LindaSummer closed 6 months ago

LindaSummer commented 7 months ago

Issue

297

We need to add an auto merge background task with cron expression scheduler.

Proposed Changes

LindaSummer commented 7 months ago

Hi @roseduan, please take a look. Thanks. 😊

I have one problem when writing unit test for auto merge.

I don't know how to check whether merge action has been operated without an internal flag.

I think I should add such a unit test for case coverage.

Best Regards

roseduan commented 7 months ago

Hi @roseduan, please take a look. Thanks. 😊

I have one problem when writing unit test for auto merge.

I don't know how to check whether merge action has been operated without an internal flag.

I think I should add such a unit test for case coverage.

Best Regards

Thanks for your PR. What does 'internal flag' mean?

Merge operation will discard all invalid data, and rewrite all valid data into new data files. So maybe you can check the valid keys, for example, you put 10 keys, and delete 5 keys, after merge, all keys num will be 5.

LindaSummer commented 6 months ago

Hi @roseduan, please take a look. Thanks. 😊 I have one problem when writing unit test for auto merge. I don't know how to check whether merge action has been operated without an internal flag. I think I should add such a unit test for case coverage. Best Regards

Thanks for your PR. What does 'internal flag' mean?

Merge operation will discard all invalid data, and rewrite all valid data into new data files. So maybe you can check the valid keys, for example, you put 10 keys, and delete 5 keys, after merge, all keys num will be 5.

Hi @roseduan , thanks very much for your warm suggestion.

I'm sorry for lack of explanation for 'internal flag'.

Previously I wanted to add a flag for merge actions and write unit test with it. But it makes no sense for production code.

I'll use keys' total count for unit test.

Happy New Year! 🎉

Best Regards

LindaSummer commented 6 months ago

Hi @roseduan, please take a look. Thanks. 😊 I have one problem when writing unit test for auto merge. I don't know how to check whether merge action has been operated without an internal flag. I think I should add such a unit test for case coverage. Best Regards

Thanks for your PR. What does 'internal flag' mean?

Merge operation will discard all invalid data, and rewrite all valid data into new data files. So maybe you can check the valid keys, for example, you put 10 keys, and delete 5 keys, after merge, all keys num will be 5.

Hi @roseduan , please take a look.

I have use datafile's key total count to make unit test.

It works now! Thanks very much for your suggestion.

roseduan commented 6 months ago

Thanks, the CI job timeout is 10 minutes, please fix the unit tests.

There are some other failures:

image
LindaSummer commented 6 months ago

Thanks, the CI job timeout is 10 minutes, please fix the unit tests.

There are some other failures

Hi @roseduan , sorry for forgot adding cleanup code for my unit tests.

I have ran this commit in my own repo's GitHub Actions.

Please take a look.

Thanks very much, Best Regards

roseduan commented 6 months ago

Thanks for your great work, looking forward to your next PR!

LindaSummer commented 6 months ago

Thanks for your great work, looking forward to your next PR!

Thanks for your warm help.

I'm satisfied to make some contribution. 😊

Best Regards