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

improve mfs republisher #754

Merged
merged 2 commits into from
Dec 13, 2024
Merged

improve mfs republisher #754

merged 2 commits into from
Dec 13, 2024

Conversation

gammazero
Copy link
Contributor

  • Get updated values while retrying to publish
  • Prefer Close to lifecycle context
  • Do not require call to Start separate from New

- Get updated values while retrying to publish
- Prefer Close to lifecycle context
- Do not require call to Start separate from New
@gammazero gammazero requested a review from a team as a code owner December 13, 2024 02:31
Copy link

codecov bot commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 49.15254% with 30 lines in your changes missing coverage. Please review.

Project coverage is 60.38%. Comparing base (0c321cc) to head (cfda17c).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
mfs/repub.go 49.12% 28 Missing and 1 partial ⚠️
mfs/root.go 50.00% 1 Missing ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #754      +/-   ##
==========================================
- Coverage   60.38%   60.38%   -0.01%     
==========================================
  Files         245      245              
  Lines       31063    31081      +18     
==========================================
+ Hits        18758    18768      +10     
- Misses      10634    10644      +10     
+ Partials     1671     1669       -2     
Files with missing lines Coverage Δ
mfs/root.go 36.98% <50.00%> (-3.53%) ⬇️
mfs/repub.go 67.44% <49.12%> (-8.75%) ⬇️

... and 11 files with indirect coverage changes

Copy link
Contributor

@hsanjuan hsanjuan left a comment

Choose a reason for hiding this comment

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

part of me wonders if something similar cannot be achieved with a single timer that is reset for long/short intervals depending how the publishing went.

mfs/repub.go Outdated Show resolved Hide resolved
@gammazero
Copy link
Contributor Author

part of me wonders if something similar cannot be achieved with a single timer that is reset for long/short intervals depending how the publishing went.

As far as I can see both timers are needed. It like when batching by count: you need to know both the batch size and how many values are in the batch so far. This is batching, but with time.

The quick timer (waiting for additional results) keeps getting reset when another result arrives. If new results are coming in continuously, resetting quick timer each time, there needs to be another timer running that will trigger an update at some point even though new values keep arriving.

In other words, the quick timeout means there are no more values to put into the "batch", so do update. The long timeout means there are that the "batch" is full, so do update, even though there are still values (no quick timeout yet) arriving.

@gammazero gammazero merged commit 058422d into main Dec 13, 2024
15 checks passed
@gammazero gammazero deleted the improve-mfs-republisher branch December 13, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants