-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Kafka-connect: Handle namespace creation for auto table creation #10186
Conversation
@@ -47,7 +54,7 @@ public class IcebergWriterFactoryTest { | |||
@ValueSource(booleans = {true, false}) | |||
@SuppressWarnings("unchecked") | |||
public void testAutoCreateTable(boolean partitioned) { | |||
Catalog catalog = mock(Catalog.class); | |||
Catalog catalog = mock(InMemoryCatalog.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to change it to some catalog that supports namespace creation. As the code now calls createNamespaceIfNotExist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not rely on InMemoryCatalog API here. You could do something like this instead, Catalog catalog = mock(Catalog.class, withSettings().extraInterfaces(SupportsNamespaces.class));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
...ct/kafka-connect/src/test/java/org/apache/iceberg/connect/data/IcebergWriterFactoryTest.java
Outdated
Show resolved
Hide resolved
Namespace namespace = Namespace.of(Arrays.copyOfRange(levels, 0, index + 1)); | ||
try { | ||
((SupportsNamespaces) catalog).createNamespace(namespace); | ||
} catch (AlreadyExistsException ex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We went the other way with Flink: #7795
Can we also swallow the exception where the user doesn't have permission to create a namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed if the auto create table is enabled, means user has permission to create tables. So, They can create Namespaces also. But that assumptions can be wrong for some catalogs. Let me catch ForbiddenException
also for ignoring it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @ajantha-bhat 👍
Any concerns @bryanck? We could also make this configurable through a flag, but I don't think we want to overcomplicate things.
for (int index = 0; index < levels.length; index++) { | ||
Namespace namespace = Namespace.of(Arrays.copyOfRange(levels, 0, index + 1)); | ||
try { | ||
((SupportsNamespaces) catalog).createNamespace(namespace); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can cause a ClassCastException so we should check that the catalog implements SupportsNamespaces first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
TableIdentifier.of(Namespace.of("foo1", "foo2", "foo3"), "bar"); | ||
Schema schema = new Schema(Types.NestedField.required(1, "id", Types.StringType.get())); | ||
|
||
try (InMemoryCatalog catalog = new InMemoryCatalog()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer using the ArgumentCaptor approach, rather than relying on InMemoryCatalog. This is consistent with the auto create test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. Updated the existing testcase
ping @bryanck |
Thanks for the contribution @ajantha-bhat , it looks good! |
Some catalogs like Inmemory, Hive, Nessie, REST catalog expects the namespace to be present before creating the table. Hence, the auto table creation feature will not work currently for these catalogs if the namespace is missing.
Added a support to create the missing namespace and its parents while auto creating the table.