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

ts: don't send DeleteRange requests for time series with nothing to p… #11762

Merged
merged 1 commit into from Dec 1, 2016

Conversation

spencerkimball
Copy link
Member

@spencerkimball spencerkimball commented Dec 1, 2016

…rune

All the information was there, just needed to be put to use. This isn't
a huge deal in practice, but eliminates sending DeleteRange requests for
time series data which isn't ready for pruning.


This change is Reviewable

@mrtracy
Copy link
Contributor

mrtracy commented Dec 1, 2016

:lgtm:


Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


pkg/ts/pruning.go, line 118 at r1 (raw file):

		// Skip this time series if there's nothing to prune. We check the
		// oldest (first) time series record's timestamp against the
		// pruning threshold.

nit: You could also use the computeThresholds method for this.

// once at start of "findTimeSeries"
thresholds = computeThresholds(timestamp.WallTime)

// When deciding to skip
if threshold, ok := thresholds[res]; !ok || threshold > tsNanos

That mirrors what is done in the pruneTimeSeries method.


Comments from Reviewable

@spencerkimball
Copy link
Member Author

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


pkg/ts/pruning.go, line 118 at r1 (raw file):

Previously, mrtracy wrote…

nit: You could also use the computeThresholds method for this.

// once at start of "findTimeSeries"
thresholds = computeThresholds(timestamp.WallTime)

// When deciding to skip
if threshold, ok := thresholds[res]; !ok || threshold > tsNanos

That mirrors what is done in the pruneTimeSeries method.

Done.


Comments from Reviewable

…rune

All the information was there, just needed to be put to use. This isn't
a huge deal in practice, but eliminates sending DeleteRange requests for
time series data which isn't ready for pruning.
@spencerkimball spencerkimball force-pushed the spencerkimball/avoid-delrange-on-pruning branch from 2dfa4f5 to b65588e Compare December 1, 2016 22:03
@spencerkimball spencerkimball merged commit ccea68c into master Dec 1, 2016
@spencerkimball spencerkimball deleted the spencerkimball/avoid-delrange-on-pruning branch December 1, 2016 22:24
@tamird
Copy link
Contributor

tamird commented Dec 4, 2016

Looks like this caused some emergent flakiness in TestTimeSeriesMaintenanceQueueServer.

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.

None yet

3 participants