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

[aggregator] Update flush times after each flush #3890

Merged
merged 30 commits into from
Nov 6, 2021

Conversation

arnikola
Copy link
Collaborator

@arnikola arnikola commented Oct 29, 2021

Previously flushed times were only updated after a delay; in practice
this appears to be needlessly defensive and impacts metric forwarding
times by up to mgr.flushTimesPersistEvery, which defaults to 10s; with
the tight timings involved in aggregator forwarding, this delay would
contribute to dropped metrics on aggregator deploys

Also adds an eager shutdown that waits only for the flush time update
before allowing the aggregator to shutdown, since this is the most imporant
step of the graceful shutdown period as is.

Previously flushed times were only updated after a delay; in practice
this appears to be needlessly defensive and impacts metric forwarding
times by up to `mgr.flushTimesPersistEvery`, which defaults to 10s; with
the tight timings involved in aggregator forwarding, this delay would
contribute to dropped metrics on aggregator deploys
@arnikola
Copy link
Collaborator Author

arnikola commented Nov 2, 2021

Tabling this one for now until I can verify the improvements (since the test work may be quite messy here)

@arnikola arnikola marked this pull request as draft November 2, 2021 14:42
require.Nil(t, flushTask)
require.Equal(t, 900*time.Millisecond, dur)
require.Equal(t, 0, storeAsyncCount)
}

func TestLeaderFlushManagerPrepareNoFlushWithPersistOnce(t *testing.T) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All of these tests were batching up persists until a particular timing was hit; now that persists are refactored to run independently of a Run, they are not necessary

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Re-added after bringing back the flushTimesPersistEvery option

@arnikola arnikola marked this pull request as ready for review November 4, 2021 22:13
@codecov
Copy link

codecov bot commented Nov 4, 2021

Codecov Report

Merging #3890 (4c58fb9) into master (4c58fb9) will not change coverage.
The diff coverage is n/a.

❗ Current head 4c58fb9 differs from pull request most recent head 86dc55d. Consider uploading reports for the commit 86dc55d to get more accurate results

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3890   +/-   ##
======================================
  Coverage    57.1%   57.1%           
======================================
  Files         553     553           
  Lines       63881   63881           
======================================
  Hits        36537   36537           
  Misses      24148   24148           
  Partials     3196    3196           
Flag Coverage Δ
aggregator 65.5% <0.0%> (ø)
cluster ∅ <0.0%> (∅)
collector 58.4% <0.0%> (ø)
dbnode 60.4% <0.0%> (ø)
m3em 46.4% <0.0%> (ø)
metrics 19.7% <0.0%> (ø)
msg 74.5% <0.0%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c58fb9...86dc55d. Read the comment docs.

src/aggregator/aggregator/aggregator.go Show resolved Hide resolved

closeLogger.Info("done")
return nil
// NB: closing the flush manager is the only really necessary step for
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice comment!

src/aggregator/aggregator/leader_flush_mgr.go Outdated Show resolved Hide resolved
src/aggregator/aggregator/leader_flush_mgr.go Show resolved Hide resolved
src/cmd/services/m3aggregator/config/aggregator.go Outdated Show resolved Hide resolved
src/aggregator/aggregator/leader_flush_mgr.go Outdated Show resolved Hide resolved
src/aggregator/aggregator/leader_flush_mgr.go Outdated Show resolved Hide resolved
src/aggregator/aggregator/leader_flush_mgr.go Outdated Show resolved Hide resolved
@arnikola arnikola enabled auto-merge (squash) November 6, 2021 15:31
@arnikola arnikola merged commit 08209d4 into master Nov 6, 2021
@arnikola arnikola deleted the arnikola/update-flush branch November 6, 2021 15:50
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.

4 participants