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

Fix no table is replicated when excludeTableRegex is set while includeTableRegex is nil #874

Merged
merged 7 commits into from
May 16, 2024

Conversation

gaojijun
Copy link
Contributor

@gaojijun gaojijun commented May 8, 2024

fix #873

@lance6716
Copy link
Collaborator

Thanks, please fix the lint error

  canal/canal.go:299: File is not `goimports`-ed (goimports)

And can you update the canal_test.go like TestCanalFilter to add an unit test for it?

@gaojijun
Copy link
Contributor Author

gaojijun commented May 8, 2024

Thanks, please fix the lint error

  canal/canal.go:299: File is not `goimports`-ed (goimports)

And can you update the canal_test.go like TestCanalFilter to add an unit test for it?

Done

@lance6716
Copy link
Collaborator

Hi, I mean add a test to explain your change. I manually deleted line 302~303 and the test TestIncludeExcludeTableRegex can still pass. I want a failed test without the change as issue said

@gaojijun
Copy link
Contributor Author

Hi, I mean add a test to explain your change. I manually deleted line 302~303 and the test TestIncludeExcludeTableRegex can still pass. I want a failed test without the change as issue said

Sorry, my fault. It's updated, please take a look.

@lance6716 lance6716 changed the title Fix bug when excludeTableRegex is set while includeTableRegex is nil Fix no table is replicated when excludeTableRegex is set while includeTableRegex is nil May 10, 2024
@lance6716
Copy link
Collaborator

Can you also update the comment of IncludeTableRegex and ExcludeTableRegex to explain the behaviout when either is not set?

My last concern is by Hyrum's Law we should not change the behaviour of API, but I think this change does no harm. Because the old behaviour will replicate nothing so there should be no user using it. As a user, I directly use BinlogSyncer and don't use canal package, @froot can you give your opinion?

rest lgtm

@gaojijun
Copy link
Contributor Author

// Default IncludeTableRegex and ExcludeTableRegex are empty, this will include all tables

@lance6716 Is this comment good enough? It's already in the code.

@lance6716
Copy link
Collaborator

// Default IncludeTableRegex and ExcludeTableRegex are empty, this will include all tables

@lance6716 Is this comment good enough? It's already in the code.

It means both are empty, not one of them is empty

@gaojijun
Copy link
Contributor Author

// Default IncludeTableRegex and ExcludeTableRegex are empty, this will include all tables
@lance6716 Is this comment good enough? It's already in the code.

It means both are empty, not one of them is empty

updated

@lance6716 lance6716 merged commit 014dcd7 into go-mysql-org:master May 16, 2024
13 checks passed
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.

to exclude databases with ExcludeTableRegex
2 participants