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

[AMORO-1870]: Support table filters to make AMS ignore tables that are not needed #1879

Merged
merged 19 commits into from
Aug 31, 2023

Conversation

celltobig
Copy link
Contributor

@celltobig celltobig commented Aug 23, 2023

Why are the changes needed?

Resolve #1870.
AMS has a database filter, but not a table filter.

Brief change log

  • Add table filter configuration for catalog

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before making a pull request

Documentation

  • Does this pull request introduce a new feature? (yes )
  • If yes, how is the feature documented? (documented)

@github-actions github-actions bot added module:core Core module module:ams-server Ams server module type:infra module:ams-dashboard Ams dashboard module labels Aug 23, 2023
@wangtaohz
Copy link
Contributor

Thanks for your work! You can add resolve #1879 to relate to the issue.

@wangtaohz wangtaohz linked an issue Aug 23, 2023 that may be closed by this pull request
2 tasks
@celltobig
Copy link
Contributor Author

image
image
image

@wangtaohz
Copy link
Contributor

The code looks good to me, but there are some check style issues to fix.
You could run mvn checkstyle:check locally to check the code before committing. @celltobig

@celltobig celltobig changed the title [AMORO-1870]: table filter #1870 [AMORO-1870]: Support table filters to make AMS ignore tables that are not needed #1870 Aug 24, 2023
.gitignore Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Patch coverage is 12.50% of modified lines.

Files Changed Coverage
.../netease/arctic/catalog/IcebergCatalogWrapper.java 12.50%

📢 Thoughts on this report? Let us know!.

@baiyangtx
Copy link
Contributor

Thank you for submitting your Pull Request.

I'm considering using two different configuration items to decide the database and table filters separately, is it a good style? Can we use one configuration item to solve this problem?
For example, using a package name filter instead of a regular expression, such as ods_.user_

@zhoujinsong What's your point of view?

@wangtaohz
Copy link
Contributor

It must be acknowledged that using 2 configurations cannot solve the situation where there are tables with the same name in different databases.
For example:
There are 4 tables in 2 databases, a.A,a.B,b.A,b.B. If we want to keep a.A,b.B, it is impossible to achieve it with two configurations.

Therefore, I think the introduced parameter should consider both the database name and the table name.

BTW, for compatibility reasons, the previous database.filter-regular-expression should not be immediately deleted.

@zhoujinsong
Copy link
Contributor

zhoujinsong commented Aug 28, 2023

Yes, there are indeed configuration complexities and limitations when using two expressions to filter databases and tables.

My idea is to make the configuration table.filter-regular-expression apply to both the database name and table name, like:

  • db1\..+ means tables under database db1
  • a\.A|b\.B means a.A and b.B

If designed in this way, table.filter-regular-expression can replace the existing database.filter-regular-expression, but we can keep a major version to be compatible with the old configuration.

HDYT? @wangtaohz @baiyangtx @celltobig

@wangtaohz
Copy link
Contributor

I think it's a very feasible way to do it. However, there are two small issues:

  1. The /. may be \. ? And it should be like
  • db1\..+ means tables under database db1
  • a\.A|b\.B means a.A and b.B

2.How about changing the table.filter-regular-expression to database.table.filter-regular-expression?
It may be clearer to indicate matching both the database name and table name

@zhoujinsong
Copy link
Contributor

zhoujinsong commented Aug 28, 2023

  1. The /. may be \. ? And it should be like

Sure, I have changed it in the comment.

2.How about changing the table.filter-regular-expression to database.table.filter-regular-expression? It may be clearer to indicate matching both the database name and table name

I do not like the configuration name database.table.filter-regular-expression and it may be clear enough if we expose the only configuration table.filter-regular-expression in docs and add some explanation.

@github-actions github-actions bot added the type:docs Improvements or additions to documentation label Aug 29, 2023
Copy link
Contributor

@wangtaohz wangtaohz left a comment

Choose a reason for hiding this comment

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

LGTM

@zhoujinsong zhoujinsong changed the title [AMORO-1870]: Support table filters to make AMS ignore tables that are not needed #1870 [AMORO-1870]: Support table filters to make AMS ignore tables that are not needed Aug 29, 2023
wangtaohz and others added 3 commits August 31, 2023 09:57
Co-authored-by: ZhouJinsong <zhoujinsong0505@163.com>
…talogMetaProperties.java

Co-authored-by: ZhouJinsong <zhoujinsong0505@163.com>
@zhoujinsong zhoujinsong merged commit 3600570 into apache:master Aug 31, 2023
4 of 6 checks passed
ShawHee pushed a commit to ShawHee/arctic that referenced this pull request Dec 29, 2023
…e not needed (apache#1879)

* [AMORO-1870]: table filter apache#1870 \n * add table filter

* fix check style

* fix gitignore

* fix db table filter

* fix table filter

* Update managing-catalogs.md and change docs

* fix checkstyle

* fix change table filter key

* change table filter md

* Update managing-catalogs.md

* Add Deprecated annotation

* Update docs/admin-guides/managing-catalogs.md

Co-authored-by: ZhouJinsong <zhoujinsong0505@163.com>

* Update ams/api/src/main/java/com/netease/arctic/ams/api/properties/CatalogMetaProperties.java

Co-authored-by: ZhouJinsong <zhoujinsong0505@163.com>

* fix check style

---------

Co-authored-by: 刘政 <zhengliu@gaojihealth.com>
Co-authored-by: wangtaohz <103108928+wangtaohz@users.noreply.github.com>
Co-authored-by: wangtao <wangtao3@corp.netease.com>
Co-authored-by: ZhouJinsong <zhoujinsong0505@163.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:ams-dashboard Ams dashboard module module:ams-server Ams server module module:core Core module type:docs Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Support table filters to make AMS ignore tables that are not needed
4 participants