pghoard icon indicating copy to clipboard operation
pghoard copied to clipboard

Ensure receivexlog respects new configuration values after SIGHUP

Open nicois opened this issue 3 years ago • 5 comments

This modifies the receivexlog to use the updated configuration settings following a SIGHUP

Why this way

When SIGHUP is issued, self.config is refreshed in all threads, but without this change, the new values are disregarded. This change ensures the updated config object is always used.

nicois avatar May 17 '22 00:05 nicois

@alanfranz-at-work the tests are failing even when run against a "null" change: that is, my original intended commit, along with a revert commit. It appears there is a problem with version pinning in the tests, leading to these failures. I'll see if there is a simple fix to resolve this problem.

Eyeballing the code, the use of these value in this python script is thread-safe: these values are each used for stateless operations, primarily determining whether to stop or continue operation. I can add a couple of simple unit tests, one the existing tests are green again.

nicois avatar May 18 '22 23:05 nicois

@alanfranz-at-work the tests are failing even when run against a "null" change: that is, my original intended commit, along with a revert commit. It appears there is a problem with version pinning in the tests, leading to these failures. I'll see if there is a simple fix to resolve this problem.

Checking this.

Eyeballing the code, the use of these value in this python script is thread-safe: these values are each used for stateless operations, primarily determining whether to stop or continue operation. I can add a couple of simple unit tests, one the existing tests are green again.

We cannot risk that a future change will fail and leave us with an inconsistent state. We should make sure that all config values are updated at the same time, and that they're not updated while being used.

Example:

        if now - self.last_disk_space_check < self.disk_space_check_interval:
            return

        bytes_free = self.get_disk_bytes_free()
        if not self.receiver_paused:
            if bytes_free < self.min_disk_space:

what happens if one section of the code runs with old version of disk_space_check_interval, and the other with an updated version of min_disk_space?

Possibly nothing, but the code is prone to failure.

ghost avatar May 19 '22 06:05 ghost

Codecov Report

Merging #530 (bc42f6b) into main (f85068c) will decrease coverage by 0.09%. The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #530      +/-   ##
==========================================
- Coverage   90.96%   90.87%   -0.10%     
==========================================
  Files          30       30              
  Lines        4595     4614      +19     
==========================================
+ Hits         4180     4193      +13     
- Misses        415      421       +6     
Impacted Files Coverage Δ
pghoard/pghoard.py 84.14% <0.00%> (-0.12%) :arrow_down:
pghoard/receivexlog.py 92.92% <85.71%> (-2.20%) :arrow_down:
pghoard/fetcher.py 95.74% <0.00%> (-2.13%) :arrow_down:

codecov[bot] avatar May 19 '22 07:05 codecov[bot]

what happens if one section of the code runs with old version of disk_space_check_interval, and the other with an updated version of min_disk_space?

Possibly nothing, but the code is prone to failure.

I've updated the code so new configuration changes are only assimilated at a specific point, meaning during each cycle the configuration values will remain consistent.

nicois avatar May 31 '22 01:05 nicois

@nicois please remember to re-request review after addressing comments, otherwise this can slip out of our view.

Please fix conflicts, then we'll make sure to have this reviewed.

ghost avatar Jun 30 '22 07:06 ghost