Clear cache api #555

replay · 2017-02-27T13:11:42Z

Adds an API endpoint to clear the cache by metric name, including all the chunks of associated aggregations.

woodsaj · 2017-02-27T18:29:24Z

Does this api endpoint need to broadcast the request to all nodes in the cluster? I feel that it should, or at least should have a flag to tell it to.

replay · 2017-03-06T13:41:02Z

Interesting idea. What's the reason why you think that's necessary, because the cluster also forwards the requests between the nodes if something requested is known to be another node? So the cache clearing behavior should be consistent with that? That makes sense

woodsaj · 2017-03-15T08:50:00Z

If running a multinode cluster, and i want to delete the cache for all series that match "some.metric.*" the series that match that pattern could be spread across many nodes.

Having the delete request propagate through the whole cluster would be a requirement for exposing this feature to users who would typically have no visibility into the cluster topology, they only have a single gateway address.

Dieterbe · 2017-03-15T12:04:52Z

+1 to support propagation. but we should make it optional. I suspect often enough cache clearing will be done by an operator who wants to only do it to 1 instance, to diagnose or try to fix a problem, or the operator may want to do it to different instances at different times, instead of all at once.

replay · 2017-09-08T14:20:51Z

@Dieterbe @woodsaj took a while, but this would be ready to review again

Dieterbe · 2017-09-08T16:20:55Z

it seems to make sense to be able to delete the entire cache, currently it can only be done for series matching the pattern, but there is no pattern to mach all series (correct me if i'm wrong), so maybe allow for pattern "" or "*" and delete entire cache in that case?

Dieterbe · 2017-09-08T16:25:07Z

api/ccache.go

+			}
+		}
+	}
+	response.Write(ctx, response.NewJson(200, res, ""))


if the peers reported a bunch of errors, or could not connect to peers, we should probably not report 200 ok

Dieterbe · 2017-09-08T16:31:36Z

seems like something went wrong with the rebase. it's showing a bunch of changes that seem unrelated to this PR, like moving cluster stuff around. can you amend the commit so that the diff is only what is supposed to change

replay · 2017-09-11T12:56:58Z

@Dieterbe I think you're right that it would make sense to add a way to clear the whole cache, but I think using "*" is kind of risky because that's supposed to have a specific meaning which is different. So I'd rather use something like "**"

Dieterbe · 2017-09-11T13:46:05Z

** ? why not just support empty pattern then?

replay · 2017-09-11T13:47:26Z

@Dieterbe because i think that "" could easily be specified by accident, that's all :)

replay · 2017-09-20T12:21:18Z

@Dieterbe is there something more that i need to do to make this mergable? (apart from rebasing once again if the context cancelling get merged)

replay · 2017-12-18T11:13:36Z

@Dieterbe good news, because I'm sure you're looking for more PRs to review, this one would be ready again^^
It also hides nodes away behind an interface in order to make them mockable, that was necessary because I wanted to add a test to test propagation of the cache deletes.
It also makes the chunk cache aware of which archives belong to which raw metric. That's necessary because when we delete the metric xyz out of the cache we'll also want to delete all the related archives like xyz_600_sum.

Dieterbe · 2017-12-29T12:50:38Z

api/models/ccache.go

+func (cd CCacheDelete) TraceDebug(span opentracing.Span) {
+}
+
+type CCacheDeleteResp struct {


the number of errors is interesting, but much more useful I think would be the first error encountered (for each node)

replay · 2018-01-03T09:54:57Z

@Dieterbe this would be ready to review again

Dieterbe · 2018-01-03T14:34:00Z

api/ccache_test.go

+
+	if respParsed.DeletedSeries != delSeries || respParsed.DeletedArchives != delArchives {
+		t.Fatalf("Expected %d series and %d archives to get deleted, but got %d and %d", delSeries, delArchives, respParsed.DeletedSeries, respParsed.DeletedArchives)
+	}


i'm confused about why we tell the cache to return these fake, made up numbers and then check them, while we also check the real numbers above. can this be simplified ? for example have the mockcache return that deleted series == deleted metric keys, and deleted archives is maybe pinned to 3x the number of deleted keys? does this idea make sense? if not, add a comment somewhere that explains this please

This is only testing the request handler, not the cache. It's testing one request with propagation disabled (TestMetricDelete) and one with propagation enabled (TestMetricDeletePropagation). I'm not sure how creating additional rules like archives = 3 * series would simplify things, that would rather just make it more complicated because in order to understand the test a reader would then first need to be aware of this rule.
Maybe I should rename the tests to TestMetricDeleteRequestHandlerWithoutPropagation() and TestMetricDeleteRequestHandlerWithPropagation?

it's confusing because the mock cache claims to have deleted a number series and archives that has nothing to do with the delete request we actually issued on it. the numbers don't seem to make sense. we assert that it only received 1 metric key delete , then how could any cache have deleted 3 series if there was only 1 key? i know it's a mock and not a real cache, but the numbers should make more sense I think.

that would clear things up if the DelMetric method took patterns.
but according to the argument names or the interface function and its implementations
(and the docs for CCache.DelMetric) it only takes 1 metric key, not a pattern.

i found a bug while looking at that just now: b168558

Dieterbe · 2018-01-03T14:42:21Z

cluster/manager.go

+	return c.thisNode()
+}
+
+func (c *MemberlistManager) thisNode() HTTPNode {


why do we need this private function if we already had the public one that did the same?

right, that's not necessary anymore, was only necessary in some previous version 👍

Dieterbe · 2018-01-03T16:47:30Z

mdata/cache/ccache_metric.go

@@ -19,6 +19,8 @@ type CCacheMetric struct {

 	// the list of chunk time stamps in ascending order
 	keys []uint32
+
+	RawMetric string


as i have mentioned before, our codebase is very GC-heavy. we need to pay attention to how many pointers we keep active. and a string has a pointer. instead of keeping the entire RawMetric string for each CCacheMetric why not just keep 1 uint8 which holds the number to be subtracted from the key len to substring and get the raw key? it'll go easier on GC and save us memory as well

(we could also store the len of the raw string in a uint8 but then we're capped to a key len of 255 which some metrics will want to exceed. but a suffix can't be longer than 255 chars, we know that for sure)

Dieterbe · 2018-01-03T17:23:05Z

@replay another round of comments and a few small commits. please have a look :)

replay force-pushed the clear_cache_api branch 4 times, most recently from 4194765 to 50925b8 Compare March 27, 2017 17:43

replay force-pushed the clear_cache_api branch from 37964d6 to 41c2188 Compare March 29, 2017 13:48

replay force-pushed the clear_cache_api branch 3 times, most recently from b93a8d1 to edf7987 Compare September 8, 2017 07:22

replay requested review from woodsaj and Dieterbe September 8, 2017 08:39

replay force-pushed the clear_cache_api branch from 7a60638 to 91675a2 Compare September 8, 2017 13:26

Dieterbe reviewed Sep 8, 2017

View reviewed changes

replay force-pushed the clear_cache_api branch from 91675a2 to e5149ef Compare September 11, 2017 10:31

replay force-pushed the clear_cache_api branch from 8e64d9a to c2ee3be Compare September 11, 2017 14:58

replay force-pushed the clear_cache_api branch 3 times, most recently from 66de456 to 0aac486 Compare December 18, 2017 09:37

Dieterbe reviewed Dec 29, 2017

View reviewed changes

replay added 4 commits January 3, 2018 17:30

remove type CCDelMetricResult

e3a4980

do not need to make slice

9458c60

test if cache still works after resetting it

510a235

minor changes according to PR comments

13b8874

replay force-pushed the clear_cache_api branch from 73f3ad5 to 13b8874 Compare January 3, 2018 09:30

improve tests

979bea4

replay force-pushed the clear_cache_api branch from bdde822 to 979bea4 Compare January 3, 2018 09:53

Dieterbe added 2 commits January 3, 2018 11:12

optimize lru delete

3a21035

sensible param names for mockCache

f54d2ba

Dieterbe force-pushed the clear_cache_api branch from f545bc4 to f54d2ba Compare January 3, 2018 10:25

Dieterbe reviewed Jan 3, 2018

View reviewed changes

small CCache.Add optimization

7322dd1

Dieterbe force-pushed the clear_cache_api branch from 75b2218 to a0f35fa Compare January 3, 2018 17:17

Dieterbe added 3 commits January 3, 2018 18:19

comment

29fa432

bit more post-Reset testing

b9b25bc

small comment tweaks

5f49ca0

Dieterbe force-pushed the clear_cache_api branch from a0f35fa to 5f49ca0 Compare January 3, 2018 17:19

replay force-pushed the clear_cache_api branch from 521206c to 0ec10db Compare January 4, 2018 07:29

replay added 2 commits January 4, 2018 17:12

remove one pointer per cached metric

db096fd

remove unnecessary method thisNode()

dfa58d3

replay force-pushed the clear_cache_api branch from 0ec10db to dfa58d3 Compare January 4, 2018 09:13

fix bug in deleting by name and fix tests

b168558

replay force-pushed the clear_cache_api branch from 58d279a to b168558 Compare January 4, 2018 10:12

Dieterbe approved these changes Jan 4, 2018

View reviewed changes

Dieterbe merged commit a0a00c4 into master Jan 4, 2018

Dieterbe mentioned this pull request Apr 11, 2018

Membersforquery fixes2 #884

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear cache api #555

Clear cache api #555

replay commented Feb 27, 2017 •

edited

Loading

woodsaj commented Feb 27, 2017

replay commented Mar 6, 2017 •

edited

Loading

woodsaj commented Mar 15, 2017 •

edited

Loading

Dieterbe commented Mar 15, 2017

replay commented Sep 8, 2017

Dieterbe commented Sep 8, 2017

Dieterbe Sep 8, 2017

Dieterbe commented Sep 8, 2017

replay commented Sep 11, 2017 •

edited

Loading

Dieterbe commented Sep 11, 2017

replay commented Sep 11, 2017

replay commented Sep 20, 2017 •

edited

Loading

replay commented Dec 18, 2017 •

edited

Loading

Dieterbe Dec 29, 2017

replay commented Jan 3, 2018

Dieterbe Jan 3, 2018

replay Jan 4, 2018

Dieterbe Jan 4, 2018

Dieterbe Jan 4, 2018

replay Jan 4, 2018

Dieterbe Jan 3, 2018

replay Jan 4, 2018

replay Jan 4, 2018 •

edited

Loading

Dieterbe Jan 3, 2018 •

edited

Loading

Dieterbe Jan 3, 2018

replay Jan 4, 2018

Dieterbe commented Jan 3, 2018

Clear cache api #555

Clear cache api #555

Conversation

replay commented Feb 27, 2017 • edited Loading

woodsaj commented Feb 27, 2017

replay commented Mar 6, 2017 • edited Loading

woodsaj commented Mar 15, 2017 • edited Loading

Dieterbe commented Mar 15, 2017

replay commented Sep 8, 2017

Dieterbe commented Sep 8, 2017

Choose a reason for hiding this comment

Dieterbe commented Sep 8, 2017

replay commented Sep 11, 2017 • edited Loading

Dieterbe commented Sep 11, 2017

replay commented Sep 11, 2017

replay commented Sep 20, 2017 • edited Loading

replay commented Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

replay commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

replay Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

Dieterbe Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dieterbe commented Jan 3, 2018

replay commented Feb 27, 2017 •

edited

Loading

replay commented Mar 6, 2017 •

edited

Loading

woodsaj commented Mar 15, 2017 •

edited

Loading

replay commented Sep 11, 2017 •

edited

Loading

replay commented Sep 20, 2017 •

edited

Loading

replay commented Dec 18, 2017 •

edited

Loading

replay Jan 4, 2018 •

edited

Loading

Dieterbe Jan 3, 2018 •

edited

Loading