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

Periodical Snapshotting #161

Closed
thevaizman opened this issue Jun 18, 2022 · 10 comments · Fixed by #250
Closed

Periodical Snapshotting #161

thevaizman opened this issue Jun 18, 2022 · 10 comments · Fixed by #250
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Milestone

Comments

@thevaizman
Copy link

Would love to see auto-snapshotting of the RDB file in DF.
Currently I can use SAVE/BGSAVE but the option to configure automatic snapshotting - i.e SAVE 60 1000 is non-existent.
Would love to replace redis with DF but I don't want to take the risk of data loss or the overhead of having an external job to take care of periodically dumping the data to disk.

Is there a plan to support this feature in the future?

@romange
Copy link
Collaborator

romange commented Jun 18, 2022

yes, we can support it, though unlikely we gonna use the same spec as redis.

btw, DF saves timestamped files by default, though it's possible to override it, and use a single snapshot file like with Redis.
What would you choose? timestamped files will require some sort of garbage collection configured externally, otherwise you will find yourself out of disk space. In addition, if you run your Redis/DF in a cloud, I might want to upload your rdb snapshots to cloud storage.

@thevaizman
Copy link
Author

thevaizman commented Jun 18, 2022

Thanks for your response!

I actually DON'T like the redis spec for the periodical snapshotting, I just used it as an example to explain the feature 😄
I would prefer a runtime arg that I can supply to DF that will indicate when to dump to disk (e.g --snapshot-interval=60 will dump every 60 seconds.

In terms of naming the snapshots, I personally choose to override the timestamps as I only want the most recent dump so I run my instance of DF with --dbfilename=dump.rdb.
You do make a valid point about saving multiple snapshots in a cloud storage to enable restoring to some point in time, but I'm taking a wild guess here that DF isn't going to actually support the upload operation to the cloud any time soon (and rightly so 😉 ) and so the user will have to spin up some external component that will automate this. Therefore, I don't see any point in using the timestamps on DF's end but just let the user handle the naming externally.

@romange
Copy link
Collaborator

romange commented Jul 9, 2022

I think that the simplest and most versatile approach would be to adopt a glob-based spec.
For example "10:00" would match 10am, but "*:00" would match every hour.
I think periodic configuration does not fit the use case, where one wants to snapshot during low-load hours.

@thevaizman
Copy link
Author

Sounds reasonable to me. Although you could argue that during high-load hours there's a bigger risk of losing data due to crashes/failures, which is why I tend to like the periodic configuration of redis.
But as long as we can use something like *:00, I believe this is sufficient for most use-cases.

@romange
Copy link
Collaborator

romange commented Jul 10, 2022

👍🏼

So, the task is:

  1. To introduce a flag save_schedule or similar in server_family.cc
  2. If the flag is not empty, to parse it on a startup and see if it fits the glob spec to match HH:MM 24h time. We probably should not crash on incorrect value but output error log and ignore.
  3. If everything is ok we should start a fiber that sleeps in a loop every 20s. (20s is enough detailed so we could catch every minute when we drift).
  4. once the fiber wakes it should check for the current time and match it with the spec. if it fits, call DoSave() function.
  5. DoSave requires a transaction object. You can create in the calling fiber. See Reload(...) function in debugcmd.cc for example.
  6. I do not see how we can test it easily in unit tests, unfortunately. However, I introduced a pytest framework under tests/pytest. We should add a test there that checks this behavior. However, this item probably depends on improve our pytest framework #199 .

@romange romange added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Jul 10, 2022
@Nike682631
Copy link

@romange Is this issue available to take up?

@romange
Copy link
Collaborator

romange commented Aug 11, 2022

This one requires deep knowledge of DragonflyDB architecture to do correctly. Lets start with other issues for now.

@Nike682631
Copy link

Sure

braydnm pushed a commit to braydnm/dragonfly that referenced this issue Aug 21, 2022
* feat(test): Added the ability to specify dragonfly cli parameters on a test basis (dragonflydb#199)

Signed-off-by: Braydn <[email protected]>
braydnm pushed a commit to braydnm/dragonfly that referenced this issue Aug 21, 2022
* feat(test): Added the ability to specify dragonfly cli parameters on a test basis (dragonflydb#199)

Signed-off-by: Braydn <[email protected]>
braydnm pushed a commit to braydnm/dragonfly that referenced this issue Aug 21, 2022
* feat(test): Added the ability to specify dragonfly cli parameters on a test basis (dragonflydb#199)

Signed-off-by: Braydn <[email protected]>
@romange romange added this to the Post-Release milestone Aug 22, 2022
@romange romange linked a pull request Aug 22, 2022 that will close this issue
braydnm added a commit to braydnm/dragonfly that referenced this issue Aug 23, 2022
Code cleanup & CONTRIBUTORS.md modifcation

Signed-off-by: Braydn <[email protected]>
braydnm added a commit to braydnm/dragonfly that referenced this issue Aug 24, 2022
Parsing and race condition fixes. Improved pytests

Signed-off-by: Braydn <[email protected]>
romange pushed a commit that referenced this issue Aug 26, 2022
* feat(server): Implemented periodic snapshotting (#161)

* feat(test): Added the ability to specify dragonfly cli parameters on a test basis (#199)

Signed-off-by: Braydn <[email protected]>

* feat(server): Implemented periodic snapshotting (#161)

Code cleanup & CONTRIBUTORS.md modifcation

Signed-off-by: Braydn <[email protected]>

* feat(server): Implemented periodic snapshotting (#161)

Parsing and race condition fixes. Improved pytests

Signed-off-by: Braydn <[email protected]>

* feat(test): Cleaned up pytest code & added documentation (#199)

- Moved tests into their own file
- Renamed test namespace to avoid naming conflicts with pytest
- Updated requirements.txt to make test environment reproducible
- Added documentation to write tests

feat(server): Updated helio submodule

Signed-off-by: Braydn <[email protected]>

Signed-off-by: Braydn <[email protected]>
Co-authored-by: Braydn <[email protected]>
@kaiserdan
Copy link

How would you snapshot every 15 minutes using this format?

@romange
Copy link
Collaborator

romange commented Aug 11, 2023

@kaiserdan we just recently introduced a new flag:
see #1599 and #1590

we will document it soon, see dragonflydb/documentation#129

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants