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

Refactor tabletmanager table filtering logic #6242

Merged
merged 1 commit into from
Jun 2, 2020

Conversation

teejae
Copy link
Contributor

@teejae teejae commented May 29, 2020

Break up tabletmanager FilterTables into a filter and application, so that filtering is reusable.

  • Add more stringent enforcement for regexp, (both prefix+suffix '/', rather than prefix-only)
  • Full set of test cases

Signed-off-by: Toliver Jue toliver@planetscale.com

}

for _, tc := range tcs {
f, err := NewTableFilter(tc.tables, tc.excludeTables, tc.includeViews)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: t.Run(...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

go/vt/mysqlctl/tmutils/schema.go Show resolved Hide resolved
Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@@ -63,94 +63,129 @@ func (tds TableDefinitions) Swap(i, j int) {
tds[i], tds[j] = tds[j], tds[i]
}

// FilterTables returns a copy which includes only whitelisted tables
type TableFilter struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: a doc comment will be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's already docs in both NewTableFilter and FilterTables, so I felt it was redundant. if you insist, i will copy the content.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to duplicate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Precommit didn't catch this? All exported names require a comment.
If it's not useful outside this package, it's better to make it unexported.
Same with NewTableFilter.

Copy link
Contributor Author

@teejae teejae May 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added comment. This is intended to actually be used outside in the mysqlctl package, and replace FilterTables, in a separate PR. Hence these are exported methods.

@teejae teejae requested a review from deepthi May 29, 2020 19:34
Signed-off-by: Toliver Jue <toliver@planetscale.com>
@teejae
Copy link
Contributor Author

teejae commented May 31, 2020

@sougou and @deepthi PTAL after comments.

@sougou sougou merged commit 3dbf472 into vitessio:master Jun 2, 2020
rohit-nayak-ps added a commit to planetscale/vitess that referenced this pull request Jun 2, 2020
Signed-off-by: Rohit Nayak <rohit@planetscale.com>
deepthi added a commit that referenced this pull request Jun 2, 2020
…aking-change

Reverting breaking change relating to filter regular expressions in #6242
@deepthi deepthi added this to the v7.0 milestone Jul 27, 2020
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.

5 participants