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

[Bug report] Catalog backend name handle the renaming of catalog #4718

Closed
jerqi opened this issue Aug 28, 2024 · 13 comments · Fixed by #4900
Closed

[Bug report] Catalog backend name handle the renaming of catalog #4718

jerqi opened this issue Aug 28, 2024 · 13 comments · Fixed by #4900
Assignees
Labels
0.7.0 Release v0.7.0 bug Something isn't working

Comments

@jerqi
Copy link
Contributor

jerqi commented Aug 28, 2024

Version

main branch

Describe what's wrong

Currenlty, Iceberg jdbc backend will use catalog name as default backend name. In my understanding, it will trigger the bug if I rename the catalog.

Error message and/or stacktrace

From the code.

catalogname
    String catalogName = properties.get("catalog-name");
    jdbcProperties.put(
        "iceberg.jdbc-catalog.catalog-name",
        properties.getOrDefault(IcebergConstants.CATALOG_BACKEND_NAME, catalogName));

How to reproduce

I have questions when I read the code

Additional context

No response

@jerqi jerqi added the bug Something isn't working label Aug 28, 2024
@jerqi
Copy link
Contributor Author

jerqi commented Aug 28, 2024

@FANNG1 Could you take a look?

@FANNG1 FANNG1 self-assigned this Aug 28, 2024
@FANNG1
Copy link
Contributor

FANNG1 commented Aug 28, 2024

thanks for reporting, I'll take it

@FANNG1
Copy link
Contributor

FANNG1 commented Aug 30, 2024

  1. The default value of CATALOG_BACKEND_NAME should be set by Gravitino server, the clients(Spark, Trino, Flink) should not care about it.
  2. Compared to catalog id , I'd like to use origin jdbc as the default catalog name, as Iceberg REST server doesn't have something like catalog id.

@diqiu50 @jerqi @jerryshao WDYT?

@diqiu50
Copy link
Contributor

diqiu50 commented Aug 30, 2024

What are the problems with using jdbc as the default catalog name?

@FANNG1
Copy link
Contributor

FANNG1 commented Aug 30, 2024

What are the problems with using jdbc as the default catalog name?

The two catalogs with the same JDBC URI will share the same table.

@diqiu50
Copy link
Contributor

diqiu50 commented Aug 30, 2024

It feels like this is not a reasonable usage.

@diqiu50
Copy link
Contributor

diqiu50 commented Aug 30, 2024

If the user wants to use one MySQL, at least the database name for Iceberg catalogs should not be the same

@FANNG1
Copy link
Contributor

FANNG1 commented Aug 30, 2024

database is included in the uri for JDBC backend.

@diqiu50
Copy link
Contributor

diqiu50 commented Aug 30, 2024

Why do two Iceberg catalogs need to use the same database? Do users expect these two catalogs to be able to see each other's tables?

@FANNG1
Copy link
Contributor

FANNG1 commented Aug 30, 2024

Why do two Iceberg catalogs need to use the same database?

Maybe used for test? I'm not sure

Do users expect these two catalogs to be able to see each other's tables?

I'm not sure, What do you think?

@diqiu50
Copy link
Contributor

diqiu50 commented Sep 2, 2024

I think it maybe used for test.

@FANNG1
Copy link
Contributor

FANNG1 commented Sep 2, 2024

@diqiu50 , could you summarize your point about the proposed changes?

@diqiu50
Copy link
Contributor

diqiu50 commented Sep 3, 2024

I think that if the user sets the catalog.backend.name, it should use the user's setting; otherwise, it should use the default

@jerryshao jerryshao added 0.6.1 0.7.0 Release v0.7.0 labels Sep 23, 2024
github-actions bot pushed a commit that referenced this issue Sep 23, 2024
…name to handle the renaming of catalog (#4900)

### What changes were proposed in this pull request?

1. Spark,Trino, Iceberg catalog and Iceberg REST server use
`getCatalogBackendName` to get catalog backend name.
2. change the default backend name to catalog backend, like `jdbc`, it
will not change after rename.

### Why are the changes needed?

Fix: #4718 

### Does this PR introduce _any_ user-facing change?
Yes

### How was this patch tested?
1. create a jdbc catalog with catalog backend name, check whether can
see the schema after rename
1. create a jdbc catalog without catalog backend name, check whether can
see the schema after rename
jerryshao pushed a commit that referenced this issue Sep 23, 2024
…name to handle the renaming of catalog (#4986)

### What changes were proposed in this pull request?

1. Spark,Trino, Iceberg catalog and Iceberg REST server use
`getCatalogBackendName` to get catalog backend name.
2. change the default backend name to catalog backend, like `jdbc`, it
will not change after rename.

### Why are the changes needed?

Fix: #4718 

### Does this PR introduce _any_ user-facing change?
Yes

### How was this patch tested?
1. create a jdbc catalog with catalog backend name, check whether can
see the schema after rename
1. create a jdbc catalog without catalog backend name, check whether can
see the schema after rename

---------

Co-authored-by: FANNG <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.7.0 Release v0.7.0 bug Something isn't working
Projects
None yet
4 participants