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

Implement TTL #6

Open
hirak99 opened this issue Dec 4, 2022 · 5 comments
Open

Implement TTL #6

hirak99 opened this issue Dec 4, 2022 · 5 comments
Labels
enhancement New feature or request

Comments

@hirak99
Copy link
Owner

hirak99 commented Dec 4, 2022

There should be a way to specify TTL, either in config or via commandline (commandline takes precedence).

Any snaps beyond TTL should be considered as expired, and cleaned up on next scheduled run.

Snaps having TTL should show it on yabsnap list.

@hirak99
Copy link
Owner Author

hirak99 commented Jan 4, 2025

Here are all the features we should have for TTL -

  • Optional specification of TTL during user snapshot: yabsnap create --ttl TTL
  • Allow changing the TTL of existing snapshots: yabsnap set-ttl TARGET --ttl TTL
  • The value for TTL should use human_interval.parse_to_secs(...), and thus allow things like "1 day", "2 months", etc.

During yabsnap list -

  • Add another column to show the TTL.

During scheduled run (yabsnap internal-cronrun) -

  • Any snapshot which has TTL, and is older the TTL, will be removed.

Building on this change, we should also provide a way to apply TTL automatically. This can replace the current system, where which snaps will be deleted is determined at runtime; and instead we determine how long a snap will be kept at creation time. This should be tracked separately - added Issue #42.

@Tql-ws1
Copy link
Contributor

Tql-ws1 commented Jan 6, 2025

May I ask if the introduction of TTL (Time To Live) was to improve the granularity and flexibility in snapshot cleanup?

Specifically, from cleaning within a fixed range for a certain source/configuration file to cleaning individual snapshots.
The enhancement in flexibility means shifting from scheduled cleanups to a combination of runtime cleanups and scheduled cleanups.

This is my understanding of how the TTL feature is implemented; should there be no issues with this?

@hirak99
Copy link
Owner Author

hirak99 commented Jan 8, 2025

May I ask if the introduction of TTL (Time To Live) was to improve the granularity and flexibility in snapshot cleanup?

Of course.

First, just adding TTL will not change cleanup process, until we also implement #42.

Just adding TTL will require it to be manually set, and will not interfere with the scheduled cleanup logic.

  • Does a scheduled snap have TTL and is now > TTL?
    • If yes, clean it up.
    • Otherwise, follow the scheduled cleanup logic.

Here on, if I understand correctly, the question is on TTL based cleanup (issue #42).

Let me first try to motivate why I am even thinking of TTL based cleanup.

The current state has a problem - the logic is complicated. I.e. at a glance it is not possible to say which of the scheduled snapshots will be cleaned up in the next hour (or next 2 hours, or next day). This makes it tough to know, for example, that a useful snapshot that you identified will not be deleted soon.

Specifically, from cleaning within a fixed range for a certain source/configuration file to cleaning individual snapshots.
The enhancement in flexibility means shifting from scheduled cleanups to a combination of runtime cleanups and scheduled cleanups.

This is my understanding of how the TTL feature is implemented; should there be no issues with this?

You are right, it can get complex if we are not careful.

Here's my proposal to keep it simple -

  • If TTL is present, we only honor TTL.
  • If TTL is not present, and it is a scheduled snapshot, we follow the current logic for cleanup.

I.e. we keep TTL and the current scheduled cleanup logic disjoint, with TTL taking precedent.

This should avoid all complexities or ambiguities.

Please LMK if this seems alright, or if you see any problems.

@Tql-ws1
Copy link
Contributor

Tql-ws1 commented Jan 8, 2025

Interesting. Before issue #42 was released and before issue #6 was supplemented, I had read the implementation description of issue #6, at that time I did not know what difference it made compared to the existing planned cleanup.

Now, out of curiosity, I asked for some clarification, and I learned what needs to be implemented within the scope of issue #6, as well as its significance. Thanks for your response!

Please LMK if this seems alright, or if you see any problems.

I can accept its significance and have not noticed any issues so far.

However, I believe that before implementing issue #6, we should first refresh the current project structure.
The main reason is that with the increase in features, it will become difficult to manage the existing file structure and content differentiation.
For example, test files and production files are placed in the same path; when implementing issue #5, I found main.py filled with many boundary conditions. If we continue adding functions directly, the cognitive complexity and management difficulty would increase further. At the same time, this approach indirectly blurs the responsibilities of functional units, making them harder to reuse in the future. What are your thoughts on this?

@hirak99
Copy link
Owner Author

hirak99 commented Jan 9, 2025

I can accept its significance and have not noticed any issues so far.

Thank you for checking #6 and #42. I detailed the algorithm to be implemented for #42 can get tricky, so it is good to design it. I laid it out here.
I'll plan to take these up in a while.

Rest of the comment is off topic to this issue #6, and deal with developmental practices. Anyone focused on this issue can feel free to skip.


For example, test files and production files are placed in the same path

I admit the decision is a carry forward from how Google organizes codes.

I know there are pros and cons, and I will switch if I hear compelling reason. At the moment I like this way of organizing however, since it has a few very good advantages.

I like this way of organizing for a few reasons however -

  • Organization: Keeping the test in the same directory makes them easier to find.
  • Discovering Tests (and Missing Tests): It makes it trivial to spot which units don't have tests, thus encouraging writing tests for each unit (or justifying why there should not be tests).
  • Refactoring: It helps in refactoring where we move units, by trivially keeping the test in same directory structure as the original file. (On the other hand, if the tests were in a different directory with same directory structure, we'd have to maintain the same structure which comes with an overload.)

found main.py filled with many boundary conditions

Would be grateful to know if there are any existing boundary conditions, and if so fix them.

If we continue adding functions directly, the cognitive complexity and management difficulty would increase further. At the same time, this approach indirectly blurs the responsibilities of functional units, making them harder to reuse in the future. What are your thoughts on this?

It would help to know how you want to redesign the main. I am assuming you mean this code sample, #5 (comment)?

If so, I am not 100% convinced of that change. I believe it is intended to replace the if-else routing logic in main().

  • I worry that the if-else logic is easier to grok than a the command registration abstraction, as the new contributors also will have to learn that now.
  • It seems to be moving the logic from one place to another. At the end of the day, some code has to store what needs to be called for which arguments. If-else router is one way and is very readable, and thus seems very applicable.
  • On the other hand, the benefit of your abstraction is that it forces the if-else logic to maintain structure and not get overly complicated later on, e.g. by us or new contributors. At this moment that seems unnatural though, and thus we can govern that ourselves.

The other big section in the main file is _parse_args(). Both currently seem to be good cognitive units. They are large code blocks, but readable and easy to understand.

So in summary, I don't feel fully convinced that this particular restructuring will reduce complexity; in fact I feel concerned that it may increase it.

What I would like however, is to move away from python's ArgumentParser, to something like typed-argument-parser. We will of course need to make sure that the change and argument parsing structure is entirely back-compatible so that we do not break any possibly existing user-scripts.

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

No branches or pull requests

2 participants