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

[tablet, queryrules] Extend query rules to check comments #7784

Merged
merged 9 commits into from
Apr 19, 2021

Conversation

setassociative
Copy link
Contributor

@setassociative setassociative commented Apr 2, 2021

Description

This PR makes three changes

  1. It introduces an optional us of fsnotify to watch for changes vs the rules file
  2. it extends rules evaluation to support comparison vs leading/trailing comments
  3. It adds support to specify leading/trailing comments as part of rule creation in rulesctl

Details: Watching the file

We use fsnotify which seems to be the generally accepted approach. There are some gymnastics necessary as watching the specific file is prone to losing track of it in the event that the file is deleted+recreated. To handle this case we watch the directory containing the file and react only when the node we receive events for matches the file we are watching.

Details: Filtering on MarginComments

This was mostly plumbing within the Rule itself + the application callstack as the tabletserver/query_executor::QueryExecutor has already baked margin comments into it during the initial phase of the Execute call.

Evaluation is similar to but not the same as the Query regexp because that is stable across query shapes (otherwise it wouldn't be the same query). For margin comments we need to defer evaluation until after the plan cache which is why Query & comments take different paths.

Test failures

The only listed failure is endtoend/tabletmanager/tablegc which seems unlikely to be related afaict.

Related Issue(s)

n/a

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required.

Deployment Notes

No changes necessary for existing deployments.

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build/CI
  • VTAdmin

@setassociative setassociative force-pushed the extend-queryrules branch 2 times, most recently from 372425e to 54898e7 Compare April 15, 2021 07:56
Signed-off-by: Richard Bailey <rbailey@slack-corp.com>
…o work as expected

Signed-off-by: Richard Bailey <rbailey@slack-corp.com>
Signed-off-by: Richard Bailey <rbailey@slack-corp.com>
Signed-off-by: Richard Bailey <rbailey@slack-corp.com>
Signed-off-by: Richard Bailey <rbailey@slack-corp.com>
Signed-off-by: Richard Bailey <rbailey@slack-corp.com>
Signed-off-by: Richard Bailey <rbailey@slack-corp.com>
Fix bug in how we attach RE to the rule in rulesctl

Signed-off-by: Richard Bailey <rbailey@slack-corp.com>
Signed-off-by: Richard Bailey <rbailey@slack-corp.com>
@rafael
Copy link
Member

rafael commented Apr 19, 2021

From our discussions, going to merge this to our branch and start playing with it in our environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Query Serving Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants