search

go-mysql-org / go-mysql

a powerful mysql toolset with Go
MIT License
4.52k stars 967 forks source link

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

Closed gaojijun closed 2 months ago

gaojijun commented 2 months ago

fix https://github.com/go-mysql-org/go-mysql/issues/873

lance6716 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

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

lance6716 commented 2 months ago

// 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 commented 2 months ago

// 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