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

upload event tracker #605

Merged
merged 1 commit into from
Jan 23, 2024
Merged

upload event tracker #605

merged 1 commit into from
Jan 23, 2024

Conversation

kathia-barahona
Copy link
Contributor

@kathia-barahona kathia-barahona commented Oct 4, 2023

About this change - What it does

Introduces UploadEventProgressTracker thread for monitoring the progress for each individual UploadEvent and logs a warning if an event has no progress for a specified period of time. It helps ensure that file uploads are making
progress and do not get stuck.

Resolves: #BF-2157

Why this way

We currently just track the amount of increments, but cannot identify if an upload got stuck.

pghoard/transfer.py Outdated Show resolved Hide resolved
@kathia-barahona kathia-barahona force-pushed the track_wal_file_upload_progress branch 4 times, most recently from e7f57aa to 8cfc277 Compare October 4, 2023 14:36
pghoard/transfer.py Outdated Show resolved Hide resolved
@kathia-barahona kathia-barahona force-pushed the track_wal_file_upload_progress branch 2 times, most recently from 68f3061 to c2bd30d Compare October 4, 2023 14:52
@kathia-barahona kathia-barahona marked this pull request as ready for review October 4, 2023 14:54
@kathia-barahona kathia-barahona marked this pull request as draft October 5, 2023 06:56
@kathia-barahona kathia-barahona force-pushed the track_wal_file_upload_progress branch from c2bd30d to c8a449d Compare October 5, 2023 08:55
@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2023

Codecov Report

Merging #605 (ab97d26) into main (ae00595) will decrease coverage by 0.21%.
Report is 2 commits behind head on main.
The diff coverage is 89.58%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #605      +/-   ##
==========================================
- Coverage   91.33%   91.13%   -0.21%     
==========================================
  Files          32       32              
  Lines        4731     4814      +83     
==========================================
+ Hits         4321     4387      +66     
- Misses        410      427      +17     
Files Coverage Δ
pghoard/basebackup/base.py 92.21% <ø> (-0.04%) ⬇️
pghoard/basebackup/chunks.py 96.81% <ø> (-0.05%) ⬇️
pghoard/basebackup/delta.py 95.25% <ø> (-0.04%) ⬇️
pghoard/pghoard.py 84.79% <100.00%> (-0.70%) ⬇️
pghoard/transfer.py 95.87% <89.01%> (-2.83%) ⬇️

... and 1 file with indirect coverage changes

@kathia-barahona kathia-barahona marked this pull request as ready for review October 5, 2023 10:44
@kathia-barahona kathia-barahona requested a review from a team October 5, 2023 10:45
pghoard/transfer.py Outdated Show resolved Hide resolved
@kathia-barahona kathia-barahona force-pushed the track_wal_file_upload_progress branch from c8a449d to 11bfcd7 Compare October 9, 2023 09:00
@kathia-barahona kathia-barahona force-pushed the track_wal_file_upload_progress branch 3 times, most recently from 7f64f28 to 7efc655 Compare October 19, 2023 09:38
@kathia-barahona kathia-barahona requested review from rdunklau and a team October 23, 2023 07:10
pghoard/transfer.py Outdated Show resolved Hide resolved
@kathia-barahona kathia-barahona force-pushed the track_wal_file_upload_progress branch from 7efc655 to f0e10b8 Compare November 21, 2023 09:13
pghoard/transfer.py Outdated Show resolved Hide resolved
pghoard/transfer.py Outdated Show resolved Hide resolved
pghoard/transfer.py Outdated Show resolved Hide resolved
pghoard/transfer.py Show resolved Hide resolved
pghoard/transfer.py Outdated Show resolved Hide resolved
@kathia-barahona kathia-barahona force-pushed the track_wal_file_upload_progress branch from f0e10b8 to 6dfb7e1 Compare November 29, 2023 16:15
Copy link
Contributor

@rikonen rikonen left a comment

Choose a reason for hiding this comment

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

One bad metrics call but otherwise looks good to me as far as concurrency and general correctness goes. I didn't check what the actual requirements here are so didn't review from the point-of-view that this makes sense in general.


self._tracked_events[file_key].increments.append(TransferIncrement(total_bytes_uploaded=total_bytes_uploaded))

self.metrics.increase(**metric_data)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it's possible for metric_data to be empty dict, should skip the call in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmm true, tho should it be...? Otherwise we are uploading files that we are not tracking 🤔

@kathia-barahona kathia-barahona force-pushed the track_wal_file_upload_progress branch 2 times, most recently from d3d2fb1 to ab97d26 Compare December 11, 2023 13:38
Vagrantfile Show resolved Hide resolved
pghoard/transfer.py Outdated Show resolved Hide resolved
@alexole
Copy link
Contributor

alexole commented Jan 3, 2024

Tiny comments, otherwise looks reasonable to me.

Introduces UploadEventProgressTracker thread for monitoring the progress for each individual UploadEvent and logs a warning if an event has no progress for a specified period of time. It helps ensure that file uploads are making
progress and do not get stuck.
@sebinsunny sebinsunny force-pushed the track_wal_file_upload_progress branch from ab97d26 to f64ffb5 Compare January 22, 2024 23:58
@sebinsunny sebinsunny requested a review from alexole January 23, 2024 00:00
@alexole alexole merged commit bcf1099 into main Jan 23, 2024
7 checks passed
@alexole alexole deleted the track_wal_file_upload_progress branch January 23, 2024 07:39
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.

5 participants