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

Update config.go #517

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dacker-soul
Copy link

Fix dump config ignore tables format

"Ignore table format is db.table " is not right, and fix the problem with ","
and please see canal/canal.go:162-166    "seps := strings.Split(ignoreTable, ",")"
@dacker-soul
Copy link
Author

dacker-soul commented Sep 4, 2020

canal/canal.go:162-166行

        for _, ignoreTable := range c.cfg.Dump.IgnoreTables {
		if seps := strings.Split(ignoreTable, ","); len(seps) == 2 {
			c.dumper.AddIgnoreTables(seps[0], seps[1])
		}
	}

忽略的表(ignoreTable),用逗号分隔 DB和table,这里好奇怪,不知道为什么,但是
canal/.go:25-26行

       // Ignore table format is db.table
	IgnoreTables []string `toml:"ignore_tables"`

要求用” . “来间隔。

@luispater
Copy link

这里原先应该就是正确的吧?
database1.table1,database2.table2,逗号是用来分割不同的表,句点是用sql语法将database和table做分割吧?

@dacker-soul
Copy link
Author

这里原先应该就是正确的吧?
database1.table1,database2.table2,逗号是用来分割不同的表,句点是用SQL语法将数据库和表做分割吧?

    for _, ignoreTable := range c.cfg.Dump.IgnoreTables {
	if seps := strings.Split(ignoreTable, ","); len(seps) == 2 {
		c.dumper.AddIgnoreTables(seps[0], seps[1])
	}
}

seps[0]是DB,seps[1]是table
根据 strings.Split(ignoreTable, ",")源代码来看,他们是通过 “,”来链接的

而注释
// Ignore table format is db.table
IgnoreTables []string toml:"ignore_tables"

要求用“.”来连接,所以自相矛盾啊

@dveeden
Copy link
Collaborator

dveeden commented Nov 8, 2024

@lance6716 could you check this PR?

@lance6716
Copy link
Collaborator

The old comment are not aligned with code. However, I think the problems are

  • Code behaviour is a bit weird. It's more common to use db.table rather than db,table
  • But we don't dare to change it, because maybe some applications already depends on the behaviour 😂
  • It does not consider database name or table name which contains . (currently contains ,)

I just have no strong ideas about fixing them. Maybe we can change the field name and toml name of

IgnoreTables []string `toml:"ignore_tables"`

to avoid application silently fails after the fix, and choose a better syntax

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.

4 participants