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

Replace reference to hard-coded default GCinterval value #1313

Merged

Conversation

icyflame
Copy link
Contributor

@icyflame icyflame commented Mar 6, 2022

By submitting this pull request, I confirm the following (please check boxes, eg [X]) Failure to fill the template will close your PR:

Please submit all pull requests against the development branch. Failure to do so will delay or deny your request

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?: 2


This hard-coded GCinterval value makes any change to the macro defining the GCinterval ineffective
when the GC runs.

For e.g., if I reduce the GC from once an hour to once every 3 minutes, the GC runs every 3 minutes,
but the mintime is set to the start of the next hour, which wipes everything from the overTime data.

I believe that this change is safe and can be merged and shipped to existing users because the
GCinterval value is not configurable and users who don't build the PiHole version that they are
running will not be affected by this change.

Signed-off-by: Siddharth Kannan mail@siddharthkannan.in

This template was created based on the work of udemy-dl.

This hard-coded GCinterval value makes any change to the macro defining the GCinterval ineffective
when the GC runs.

For e.g., if I reduce the GC from once an hour to once every 3 minutes, the GC runs every 3 minutes,
but the mintime is set to the start of the next hour, which wipes everything from the overTime data.

I believe that this change is safe and can be merged and shipped to existing users because the
GCinterval value is not configurable and users who don't build the PiHole version that they are
running will not be affected by this change.

Signed-off-by: Siddharth Kannan <mail@siddharthkannan.in>
@icyflame icyflame marked this pull request as ready for review March 6, 2022 15:42
@DL6ER
Copy link
Member

DL6ER commented Mar 11, 2022

The same comment as in #1312 applies here. Most of this was not really designed to run at any custom intervals and the lack of complains (so far) seems to have proven that basically nobody even tried it. In the ongoing Pi-hole v6.0 development, we're looking at using an in-memory SQLite3 database instead of relying heavily on our own datastructure to gain some more flexibility. This should also have taken care of these two bugs.

@DL6ER
Copy link
Member

DL6ER commented Mar 11, 2022

Screenshot from 2022-03-11 05-44-45
The base branch requires all commits to be signed. Learn more about signing commits.

Do you want to do this? If not, we can add an exception for your commits.

@DL6ER DL6ER merged commit f527e73 into pi-hole:development Mar 18, 2022
@DL6ER
Copy link
Member

DL6ER commented Mar 18, 2022

Merged unsigned commit using admin privileges.

@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-ftl-v5-15-web-v5-12-and-core-v5-10-released/54987/1

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

3 participants