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] Implement timedMetricList.FlushOffset #3292

Merged
merged 6 commits into from
Mar 16, 2021

Conversation

linasm
Copy link
Collaborator

@linasm linasm commented Feb 26, 2021

What this PR does / why we need it:
Changes timedMetricList to use fixed flush offset.

Special notes for your reviewer:
Dropping fixedOffsetFlushingMetricList interface and moving the FlushOffset() method to flushingMetricList (with ok flag) to avoid the error-prone type check in https://github.com/m3db/m3/pull/3292/files#diff-5a6baf266bd455b257e1a2f2ded077f3f2373a44628318748d2008d9ba59b9bcL284 which was masking the fact that timedMetricList was not exposing its flushOffset.

Old code:

	// Forwarded metric list has a fixed flush offset to ensure forwarded metrics are flushed
	// immediately after we have waited long enough, whereas a standard metric list has a flexible
	// flush offset to more evenly spread out the load during flushing.
	var (
		flushInterval = l.FlushInterval()
		flushOffset   time.Duration
	)
	if fl, ok := l.(fixedOffsetFlushingMetricList); ok {
		flushOffset = fl.FlushOffset()
	} else {
		flushOffset = mgr.computeFlushIntervalOffset(flushInterval)
	}

New code:

	flushInterval := l.FlushInterval()
	flushOffset, ok := l.FixedFlushOffset()
	if !ok {
		flushOffset = mgr.computeFlushIntervalOffset(flushInterval)
	}

Does this PR introduce a user-facing and/or backwards incompatible change?:
NONE

Does this PR require updating code package or user-facing documentation?:
NONE

@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

Merging #3292 (5875a94) into master (5875a94) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3292   +/-   ##
=======================================
  Coverage    72.3%    72.3%           
=======================================
  Files        1098     1098           
  Lines      102020   102020           
=======================================
  Hits        73807    73807           
  Misses      23115    23115           
  Partials     5098     5098           
Flag Coverage Δ
aggregator 77.0% <0.0%> (ø)
cluster 84.9% <0.0%> (ø)
collector 84.3% <0.0%> (ø)
dbnode 78.5% <0.0%> (ø)
m3em 74.4% <0.0%> (ø)
m3ninx 73.6% <0.0%> (ø)
metrics 19.8% <0.0%> (ø)
msg 74.3% <0.0%> (ø)
query 67.3% <0.0%> (ø)
x 80.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 5875a94...b98ac13. Read the comment docs.

@linasm linasm marked this pull request as ready for review March 9, 2021 11:09
@linasm linasm changed the title WIP [aggregator] Implement timedMetricList.FlushOffset [aggregator] Implement timedMetricList.FlushOffset Mar 9, 2021
@linasm linasm requested a review from robskillington March 9, 2021 11:32
Copy link
Collaborator

@robskillington robskillington left a comment

Choose a reason for hiding this comment

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

LGTM and definitely surprised this was not the case before

@linasm linasm merged commit e8c3db1 into master Mar 16, 2021
@linasm linasm deleted the linasm/m3agg-add-timedMetricList-FlushOffset branch March 16, 2021 15:40
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.

3 participants