[Prototype] More functional query killing 🎁

[setassociative]

Description

This PR makes two changes:

1. It introduces a timer-based poll of a rule config; this means we can dynamically update the query killing rules on a specific host; nominally this was already possible utilizing the topocustomrule but I kinda didn't love adding more stuff into the topo 🤷‍♂️

2. Expands the query killer to match against leading and trailing comments

This branch is based on our 8.0 deploy; I haven't checked to see if there are upstream refactor/merge conflicts yet

TODO

1. We could deploy this. It needs more love if we wanted to push it upstream but I wanted to stake-in-the-ground and see if folks felt is was worth doing for our build / upstream.

2. I have a README for how QueryRules work (will drop in channel) that needs to be updated with the new stuff here + productionization roadmap.

3. I need to update the rulsctl impl (cf Implements CLI tool for rule management vitessio/vitess#7712) to support the new comment filtering

DEMO

[guidoiaquinti]

Nice work! 👏 I've implemented something similar in a branch a bit ago but the query filtering/blocking was done at the vtgate layer. What do you think about that VS tablet?

[setassociative]

Nice work! 👏 I've implemented something similar in a branch a bit ago but the query filtering/blocking was done at the vtgate layer. What do you think about that VS tablet?

Structurally I think this should live in the vtgates. I took the route of extend tablet filtering bc it was already there and the refactor was small enough that I'm comfortable baking this in prod with minimal testing additional testing.

I do think we actually probably could lift this approach into the gates without much effort but I'm not a fan of the current rules structure as it limits extensibility.
In my mind each filter should meet some interface so that we can build custom implementations that can work by doing something more than regexp matching if desired. That might be over engineering or it might be glorious customization opportunities 🤷. Ideally we'd also have a vtgate filter rule impl be more fully featured than this having some ability to do query re-routing (primary->replica) and have additional matching options (which keyspace, which target tablet type, etc).

That said if your branch is in a reasonable state to ship let's do it (also happy to jump in to help w/ it if that feels like the right move) -- I approached with fairly tight constraints "limit refactoring" and "a couple of afternoons timebox" and am very much not married to it. Mostly I wanted something viable as a stepping stone to whatever our long term solution was.

[guidoiaquinti]

Structurally I think this should live in the vtgates. I took the route of extend tablet filtering bc it was already there and the refactor was small enough that I'm comfortable baking this in prod with minimal testing additional testing.

👍 LGTM

I do think we actually probably could lift this approach into the gates without much effort but I'm not a fan of the current rules structure as it limits extensibility.
In my mind each filter should meet some interface so that we can build custom implementations that can work by doing something more than regexp matching if desired. That might be over engineering or it might be glorious customization opportunities 🤷. Ideally we'd also have a vtgate filter rule impl be more fully featured than this having some ability to do query re-routing (primary->replica) and have additional matching options (which keyspace, which target tablet type, etc).

✨

That said if your branch is in a reasonable state to ship let's do it (also happy to jump in to help w/ it if that feels like the right move) -- I approached with fairly tight constraints "limit refactoring" and "a couple of afternoons timebox" and am very much not married to it. Mostly I wanted something viable as a stepping stone to whatever our long term solution was.

My branch is not in a reasonable state unfortunately. I think your approach is ✅ and we can iterate over this and maybe extend the filtering to vtgate as well in the future.

[guidoiaquinti]

Nothing against your lib ! but what about github.com/fsnotify/fsnotify ?

[setassociative]

Nothing against your lib ! but what about github.com/fsnotify/fsnotify ?

Mostly getting it to work with my thing was essentially trivial to drop in and not the interesting part of the change. I'll swap out for a tested solution before actually baking the review

[setassociative]

oh ugh. I remember thinking about this now

So the fancy way of doing this with notifications is that it basically is hard to operate on a file that gets created/renamed/deleted/doesn't exist. The dumb-as-rocks version i used in my library is totally fine with that since it's just polling.

I mostly don't have strong feelings about this and don't want to fight upstream about using my jank so I'm still going to try swapping out for fsnotify but that's the underlying reason why it might be preferred to do the simple thing instead.

[guidoiaquinti]

Minor comment but overall looks good to me

[setassociative]

This is replaced by vitessio#7784