feat(tsm): Allow for deletion of series outside default rp

[devanbenz]

Modifies logic of TSDB deletion to include filtering by retention policy

Part one of this FR is located here: influxdata/influxql#71

This is part two, InfluxQL is updated here and this adds filtering by shard's retention policy within the delete series method.

Closes https://github.com/influxdata/feature-requests/issues/175

[davidby-influx]

I think your shard copying algorithm is wrong, or I am misunderstanding it.

[gwossum]

A few comments, and a large concern related to filtering by retention policy.

[devanbenz]

This seems wrong. Is the goal to only return shards that every function returns true for, and to only return each shard once?

You are allocating the shards slice once per function. That means that you are only getting the shards which pass the last function in the function slice, correct?

Do you want to instead, walk the shards in the outer loop, and the functions in the inner loop, and append a shard only if all functions return true for that shard?

I'm thinking of something like this. Am I misunderstanding your algorithm?

for _, sh := range s.shards {
        votes := 0
	for _, fn := range fns {
                if fn == nil || fn(sh) {
                        votes++
                }                      
        }
        if votes == len(fns) {
                   shards = append(shards, sh)
        }
}

You are correct, revisiting this there is a bug in the shard filtering logic. It will effectively throw away the "database" filtered shards using the byDatabase filter then create a new slice of shards and only filter by the next function (in this case byRetentionPolicy). The logic was operating correctly on existing tests just due to the fact that all other calls to the method are only using a single filter function... introduce more and it would not filter correctly.

Furthermore, If I were to write another test where I have two or more databases with the same retention policy name this would effectively delete series across multiple databases which is not the desired functionality or current way this works. I suspect that what ever database is the currently selected database should be processing the deletes. I'll go ahead and adjust this today.

[davidby-influx]

A suggestion

[davidby-influx]

I think it would be cleaner here to actually make a composed function that calls the first shardFilter function. That way we can change byDatabase to do more or less or different things.

[davidby-influx]

Something like this:

func ComposeFilter(original func(i int) bool, addition func(i int) bool) func(i int) bool {

	return func(i int) bool { return original(i) && addition(i) }
}

[devanbenz]

I do like that suggestion, I think it looks really clean.

[gwossum]

ComposeFilter seems needlessly complicated, especially since it would get used one place.

You could just make use byDatabase in the new shardFilter definition:

shardFilter := func(sh *Shard) bool { return byDatabase(sh) && sh.retentionPolicy == measurement.RetentionPolicy }

You could then pull sh.retentionPolicy == measurement.RetentionPolicy into a byRetentionPolicy function if you wanted.

[gwossum]

I would probably change the name of shardFilter to something else because it is too similar to filterShards. My brain crossed them together the first time I read the code. Maybe change to shardFilter to sf or filter.

[davidby-influx]

We do support wildcards for measurements in DELETE. Please verify the syntax to use, and add tests including them.

[devanbenz]

We do support wildcards for measurements in DELETE. Please verify the syntax to use, and add tests including them.

Sounds good 🫡

[gwossum]

@devanbenz You could do byDatabase(database)(sh) && byRetentionPolicy(retention)(sh), but it's probably better to call byDatabase() and byRetentionPolicy() outside the lambda and assign them to variables, then use them inside the lambda. Or use @davidby-influx's ComposeFilter idea. Another way to do the compose filter would be something like:

func ComposeFilters(fns ...func(sh *Shard) bool) func(sh *Shard) bool {
   return func(sh *Shard) bool {
        for _, f := range fns {
            if !f(sh) {
                return false
            }
        }
        return true
    }
}

....

    shards := s.filterShards(ComposeFilters(byDatabase(db), byRetentionPolicy(rp)))

I find that way more readable, but that's personal preference.

I'd also be tempted to add a type ShardPredicate func(sh *Shard) bool 😁

[devanbenz]

Ah yes! @gwossum this is similar to what I was attempting to go for with my earlier design of passing in multiple functions but much much more elegant 🙏

[devanbenz]

Okay modifications have been made -- testing of wildcards as well as adding the composable function for shard filtering + I refactored other areas that is calling the function to include the composition fn

[davidby-influx]

LGTM

[gwossum]

Looking good! I still think we should handle more than one source, lest we regret it in the future.

[gwossum]

I think we decided the parser wouldn't allow more than a single source, but I think it would still be good to check for that here and return an error. There might be another way to get more than measurement (Flux? Probably not, but are you sure?). There also might be a change made to the parser that allows more than one source, and if we don't check for that possibility here that could lead to strange issues in the future that we spend weeks tracking down. Returning an error instead what make it clear what happened.

[gwossum]

LGTM