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

Document aof-disable-auto-gc in redis.conf #12249

Closed
wants to merge 1 commit into from

Conversation

sonicloong
Copy link

@sonicloong sonicloong commented May 31, 2023

It appears that currently this configuration directive is not mentioned anywhere outside of the codebase, so documenting it in redis.conf would make it more visible to users.

Copy link
Collaborator

@enjoy-binbin enjoy-binbin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, it is only used for testing at the moment, we does not have plans to expose it yet

@sonicloong
Copy link
Author

Got it. Is there any downside for disabling AOF auto GC, aside from having to manage the storage space? This feature is very helpful to our log based CDC solution, so we'd like to know if we are missing anything, and whether this can be officially exposed. @enjoy-binbin

Also IMHO even though this feature is not explicitly "exposed" in any document, it is accessible as a config directive in public releases, and users have the ability to enable/disable it through the config file. So if the feature is supposed to be experimental or for testing only, then it would be great to have a clear indication or mention regarding its experimental status, so that users would be aware that the feature might not be fully supported yet, and could potentially undergo changes or be removed in future releases.

@oranagra
Copy link
Member

i don't think it has any drawbacks other than disk (and manifest file) growth.

i see that in #9788 we explicitly said it was for testing purposes, but we didn't flag it with flag it with HIDDEN_CONFIG.
i don't recall the discussions around it and future plans, @CharlesChen888 maybe you remember something?

@CharlesChen888
Copy link
Contributor

@oranagra maybe you should ping @chenyang8094 ?

@oranagra
Copy link
Member

oranagra commented Jun 14, 2023

sorry, that's what i meant, but GH must have suggested you first and i didn't notice.

@chenyang8094
Copy link
Collaborator

chenyang8094 commented Jun 17, 2023

@sonicloong
Hello, the original intention of aof-disable-auto-gc was indeed only for testing purposes (to check if certain AOF files were actually generated and if they were deleted correctly during testing). However, I also know that the purpose of aof-disable-auto-gc is certainly not limited to this. When aof-disable-auto-gc is set to False, we can accumulate AOF datasets for backup and point-in-time recovery (PITR, which also relies on AOF's timestamp annotation feature), CDC and other data transfer tools. In a sense, aof-disable-auto-gc can indeed solve some problems (preventing Redis from automatically deleting AOF), but its disadvantages are also obvious, i.e. it cannot determine which AOFs in history state can be deleted. In fact, at Alibaba Cloud, we have implemented a more complex AOF file reference counting capability, where any component that needs to consume AOF (such as CDC) will send a command to Redis to increase the reference count of the specified AOF file (and then use a Redis command to reduce the file reference count after CDC sends the corresponding data of the AOF), and only history AOFs with a reference count of 0 will be automatically deleted. This way, we have achieved the ability to control whether each AOF is automatically deleted separately.

In general, aof-disable-auto-gc can solve your problem and Redis does not impose any restrictions, but I want to say that it is not perfect or complete. I hope my answer can help you.

@oranagra
Copy link
Member

I don't think we want to build a more advanced mechanism around this now, so the question is if we should set a HIDDEN_CONFIG flag on it, or document it and the limitations / implications of using it?
i'm leaning towards hiding it.
@yossigo WDYT?

@yossigo
Copy link
Member

yossigo commented Jun 18, 2023

@oranagra I think it should be hidden, as it was created for testing purposes and may have sharp edges as a general-purpose feature. I agree with @chenyang8094 that it can have more value, but I suggest we consider separately the right way to expose this functionality to users.

Copy link
Collaborator

@enjoy-binbin enjoy-binbin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sonicloong please do this to hide the config

-    createBoolConfig("aof-disable-auto-gc", NULL, MODIFIABLE_CONFIG, server.aof_disable_auto_gc, 0, NULL, updateAofAutoGCEnabled),
+    createBoolConfig("aof-disable-auto-gc", NULL, MODIFIABLE_CONFIG | HIDDEN_CONFIG, server.aof_disable_auto_gc, 0, NULL, updateAofAutoGCEnabled),

enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Jun 27, 2023
aof-disable-auto-gc was created for testing purposes,
to check if certain AOF files were actually generated
and if they were deletedcorrectly during testing.

So hiding it, see redis#12249 for more discussion.
@enjoy-binbin
Copy link
Collaborator

closing in favor of #12355

oranagra pushed a commit that referenced this pull request Jun 27, 2023
aof-disable-auto-gc was created for testing purposes,
to check if certain AOF files were actually generated
and if they were deletedcorrectly during testing.

So hiding it, see #12249 for more discussion.
@sonicloong
Copy link
Author

Thanks @chenyang8094 for the context and @enjoy-binbin for making the change. I was on a long break, so didn't get the chance to send a follow-up PR.

but I suggest we consider separately the right way to expose this functionality to users

It would be great to provide a production-ready option for users to opt out of AOF auto GC in future releases. Looking forward to it.

@sonicloong sonicloong deleted the aof-gc-doc branch August 10, 2023 01:36
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

Successfully merging this pull request may close these issues.

None yet

6 participants