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

Temporarily turn off the background fetch until it gets re-worked #176

Closed
wants to merge 1 commit into from

Conversation

vkuzniet
Copy link
Contributor

@vkuzniet vkuzniet commented Nov 9, 2022

Signed-off-by: Viktor Kuznietsov vkuzniet@amazon.com

Issue #, if available:

Description of changes:
Temporarily turning off the background fetch until it gets re-worked. For more details see the issue #106

Testing performed:

  • make test && make check && make integration pass
  • Deployed soci-snapshotter-grpc and soci to an EC2 instance, created an index for rethinkdb and ran the container workload in lazy loading mode. Observed no log record referring to background fetcher.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Signed-off-by: Viktor Kuznietsov <vkuzniet@amazon.com>
@vkuzniet vkuzniet requested a review from a team as a code owner November 9, 2022 16:50
Copy link
Contributor

@hanyuel hanyuel left a comment

Choose a reason for hiding this comment

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

With this change, even when a user passes in NoBackgroundFetch=false in config parameter for running the snapshotter, the background fetch will not run. I don’t think we should silently not run the bg fetch w/o any message when user explicitly sets the background fetch on. Sure the background fetch will be re-designed, but today it still can run. I suggest changing the behavior to not running the background fetch if no bg fetch config pass in from the user, and using the config from the user if provided.

@vkuzniet
Copy link
Contributor Author

vkuzniet commented Nov 9, 2022

With this change, even when a user passes in NoBackgroundFetch=false in config parameter for running the snapshotter, the background fetch will not run. I don’t think we should silently not run the bg fetch w/o any message when user explicitly sets the background fetch on. Sure the background fetch will be re-designed, but today it still can run. I suggest changing the behavior to not running the background fetch if no bg fetch config pass in from the user, and using the config from the user if provided.

It is usually on by default. There's a config option to turn it off. The problem with intelligent handling there is that the config.toml is being deserialized into config struct and there's no way to know if the user set this field or it was set by default.
If we agree that background fetcher as it is right now interferes with on-demand fetches, and it does seem so, then the easiest thing is to turn it off temporarily.

Another option would be to keep it disabled by default, e.g. name the option as toml:background_fetch_enabled, which will be set to false by default. In this case, the likelihood of someone enabling it would be much smaller.

@sbuckfelder
Copy link
Contributor

I'm comfortable with this

@vkuzniet vkuzniet closed this Nov 21, 2022
@vkuzniet vkuzniet deleted the no-bgfetch branch December 5, 2022 16:29
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

4 participants