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-3413] Optimize execution order when disposing tables #3447

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jzjsnow
Copy link
Contributor

@Jzjsnow Jzjsnow commented Feb 26, 2025

Why are the changes needed?

Currently, when adding and deleting tables from external catalogs, the tableIdentifier is added/deleted first and then the tableRuntime is added/deleted sequentially, this may result in a scenario where tableIdentifier deletion succeeds but tableRuntime deletion fails, and the following problems further occur:

  1. The web page displaying tableRuntime list for the specific catalog fails, because these tableRuntimes can not find the corresponding information in the table_identifier table;
  2. tableRuntime retained in the persistence will block the re-insertion of the table, due to the unique key conflicts.

Close #3413.

Brief change log

  • When calling disposeTable() to dispose of a table, we first remove the tableRuntime before removing the tableIdentifier. This follows the reverse process of the syncTable() method, where we first add the tableIdentifier and then add the tableRuntime.
  • When callingdispose() to dispose of a tableRuntime instance, we first unregistry metrics, then close the optimizing processes and remove the tableRuntime from persistence.

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 / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@github-actions github-actions bot added the module:ams-server Ams server module label Feb 26, 2025
@Jzjsnow Jzjsnow force-pushed the fix_table_sync_for_external_catalogs branch from 4259e5d to 3781c17 Compare February 26, 2025 07:06
@zhoujinsong
Copy link
Contributor

Thanks for the fix, this PR may resolve #3444 too.

@zhoujinsong
Copy link
Contributor

@Jzjsnow Thanks for the work!

I have a different perspective on this issue. Wouldn't it be a better approach to delete the runtime table and the identifier table together through transaction control during the dispose phase?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:ams-server Ams server module
Projects
None yet
3 participants