Repository navigation
Flink: Dynamic Sink: Create the namespace when the catalog reports it as a missing table - #18582
kumarpritam863 wants to merge 2 commits into
Conversation
mxm
left a comment
There was a problem hiding this comment.
Thanks for the PR @kumarpritam863! We usually implement changes for the newest Flink version first, then backport the changes. The changes are reasonable small, so we can skip this here.
| private boolean namespaceIsMissing(TableIdentifier identifier) { | ||
| return catalog instanceof SupportsNamespaces | ||
| && !identifier.namespace().isEmpty() | ||
| && !((SupportsNamespaces) catalog).namespaceExists(identifier.namespace()); |
There was a problem hiding this comment.
Wouldn't this cover all scenarios? Do we still need the old exists.f1 instanceof NoSuchNamespaceException check? In that case, can we inline the method?
There was a problem hiding this comment.
Thanks @mxm for the review. Yes that one check is enough and i have replaced it with an inline function call. Also I will make sure from the next time that the changes are for latest version only.
| // The lookup cannot tell a missing table from a missing namespace: the Glue catalog, for | ||
| // instance, reports a table in a namespace that does not exist as NoSuchTableException. Ask | ||
| // the catalog directly. | ||
| if (catalog instanceof SupportsNamespaces && !identifier.namespace().isEmpty()) { |
There was a problem hiding this comment.
if (exists.f1 instanceof NoSuchNamespaceException || exists.f1 instanceof NoSuchTableException) {| // The lookup cannot tell a missing table from a missing namespace: the Glue catalog, for | ||
| // instance, reports a table in a namespace that does not exist as NoSuchTableException. Ask | ||
| // the catalog directly. | ||
| if (catalog instanceof SupportsNamespaces && !identifier.namespace().isEmpty()) { |
There was a problem hiding this comment.
I think it is needed as a table identifier without a namespace is valid (TableIdentifier.parse("myTable") in the tests), and without the check HadoopCatalog.namespaceExists(Namespace.empty()) throws IllegalArgumentException: Can not create a Path from an empty string; three existing tests fail. I added a comment.
| } catch (AlreadyExistsException e) { | ||
| LOG.debug("Namespace {} created concurrently", identifier.namespace(), e); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think we must throw if the catalog doesn't implement SupportsNamespaces (else branch) or we have another type of exception.
There was a problem hiding this comment.
For a catalog without SupportsNamespaces it now throws a clear UnsupportedOperationException when the lookup reported a missing namespace
| "Cannot create the namespace of table %s: catalog %s does not support namespaces", | ||
| identifier, catalog.name()), | ||
| exists.f1); | ||
| } |
There was a problem hiding this comment.
We should also throw in case of other exceptions in exists.f1.
There was a problem hiding this comment.
done. A lookup result other than NoSuchTableException or NoSuchNamespaceException is now thrown as an IllegalStateException with the exception as the cause. Today TableMetadataCache.exists() can only return those two (anything else propagates out of refreshTable), so this is a guard for that contract, with a test that feeds it a different exception.
| // The lookup cannot tell a missing table from a missing namespace: the Glue catalog, for | ||
| // instance, reports a table in a namespace that does not exist as NoSuchTableException. Ask | ||
| // the catalog directly. | ||
| if (catalog instanceof SupportsNamespaces) { |
There was a problem hiding this comment.
This comment is misleading because it only describes an edge case. The normal case would be a race condition with other tasks which try to create the namespace at the same time.
There was a problem hiding this comment.
agreed, reworded. It now says that tasks arriving at the same time can all find the namespace missing and try to create it (the AlreadyExistsException is ignored), and that the catalog is asked instead of the lookup because catalogs differ in which exception they throw (Glue throws NoSuchTableException) and a cached answer carries none.
| if (exists.f1 != null | ||
| && !(exists.f1 instanceof NoSuchTableException) | ||
| && !(exists.f1 instanceof NoSuchNamespaceException)) { | ||
| throw new IllegalStateException( | ||
| String.format("Unexpected failure looking up table %s", identifier), exists.f1); | ||
| } |
There was a problem hiding this comment.
Let's use Precondition.checkState instead.
There was a problem hiding this comment.
The remaining check is now a Preconditions.checkState.
| } else { | ||
| throw new UnsupportedOperationException( | ||
| String.format( | ||
| "Cannot create table %s: catalog %s does not support namespaces, so the namespace of the table cannot be created if it is missing", | ||
| identifier, catalog.name()), | ||
| exists.f1); |
There was a problem hiding this comment.
Let's move this into a Precondition.checkState(catalog instanceof SupportsNamespace, ...); check before line 93. We can then remove the if condition.
There was a problem hiding this comment.
Done. The check is now the first statement in the Boolean.FALSE.equals(exists.f0) block, and the else branch is gone.
| TableIdentifier identifier, Schema schema, PartitionSpec spec, TableCreator tableCreator) { | ||
| Tuple2<Boolean, Exception> exists = cache.exists(identifier); | ||
| if (Boolean.FALSE.equals(exists.f0)) { | ||
| if (exists.f1 instanceof NoSuchNamespaceException) { |
There was a problem hiding this comment.
We still need this check. There is no need to run the code below when exists.f1 == null .
There was a problem hiding this comment.
- DynamicRecordProcessor.processRecord calls tableCache.exists() before it calls the updater. That caches the "not found" answer.
- When TableUpdater asks again, it gets the cached answer with f1 == null. Gating on f1 would skip namespace creation on the immediate-update path.
- Glue also throws NoSuchTableException for a missing namespace, not NoSuchNamespaceException.
This block only runs when the table is missing, so it runs once per table creation. A single namespaceExists call is negligible there. The new testNamespaceCreatedWhenCatalogReportsMissingNamespaceAsMissingTable primes the cache with cache.exists(...) first, as the processor does. It fails on the old code.
312234d to
180718f
Compare
… as a missing table TableUpdater created a missing namespace only when the table lookup threw NoSuchNamespaceException. Catalogs such as Glue report a table in a missing namespace as NoSuchTableException, and the processor's earlier lookup leaves a cached answer with no exception, so the namespace was never created and createTable failed on every record. Ask the catalog whether the namespace exists instead, and fail clearly when the catalog does not support namespaces.
180718f to
9bfdec2
Compare
| if (exists.f1 instanceof NoSuchNamespaceException) { | ||
| SupportsNamespaces catalogWithNameSpace = (SupportsNamespaces) catalog; | ||
| Preconditions.checkState( | ||
| catalog instanceof SupportsNamespaces, |
There was a problem hiding this comment.
What if the catalog does not implement SupportsNamespaces? I don't think we should throw then.
There was a problem hiding this comment.
Good point, that was a regression: before this PR such a catalog could still create tables, since the cast only happened inside the NoSuchNamespaceException branch. Removed the checkState. The namespace step now only runs for catalogs that implement SupportsNamespaces; others go straight to createTable.
| TableIdentifier identifier, Schema schema, PartitionSpec spec, TableCreator tableCreator) { | ||
| Tuple2<Boolean, Exception> exists = cache.exists(identifier); | ||
| if (Boolean.FALSE.equals(exists.f0)) { | ||
| if (exists.f1 instanceof NoSuchNamespaceException) { |
There was a problem hiding this comment.
If we don't use f1 anymore, we should remove it.
There was a problem hiding this comment.
Agreed, nothing reads it anymore. I removed it from TableMetadataCache.exists() altogether rather than just from the local variable, so it now returns a boolean. The EXISTS/NOT_EXISTS tuple constants went with it, and DynamicRecordProcessor no longer needs .f0. If you only meant the local variable, I can revert the cache part.
|
|
||
| @Test | ||
| void testTableCreationFailsForCatalogWithoutNamespaceSupport() { | ||
| Catalog catalog = Mockito.mock(Catalog.class); |
There was a problem hiding this comment.
Could we remove the mock? CachingCatalog.wrap(inMemoryCatalog) is a catalog without SupportsNamespaces.
There was a problem hiding this comment.
Done. The Mockito test is replaced by testTableCreatedForCatalogWithoutNamespaceSupport, which wraps an InMemoryCatalog in CachingCatalog and asserts the table gets created. It asserts the wrapper isn't a SupportsNamespaces, so it will flag it if that ever changes. The namespace is created on the inner catalog first, because InMemoryCatalog rejects tables in a missing namespace.
A catalog that does not implement SupportsNamespaces could create tables before the namespace fix, so failing for it is a regression. Skip the namespace step for such catalogs instead of throwing. TableMetadataCache.exists() no longer returns the lookup exception, as nothing reads it any more, so it returns a boolean.
The Flink dynamic sink's
TableUpdater.findOrCreateTablecreates a missing namespace only when the table lookup throwsNoSuchNamespaceException. The Glue catalog never does:GlueTableOperations.getGlueTablereturnsnullonEntityNotFoundException, so a table in a database that does not exist looks like a table that does not exist yet andloadTablethrowsNoSuchTableException. The namespace is never created, andcreateTablethen fails with(
GlueTableOperations,EntityNotFoundException->NotFoundException). A job that writes to a database that does not exist yet fails on every record and restarts. Seen on a cluster with a Flink dynamic sink writing to a new Glue database; the task manager log carried the message above.Change
findOrCreateTableno longer relies on the lookup to report a missing namespace. Before creating a table it checks, withPreconditions.checkState, thatNoSuchTableExceptionorNoSuchNamespaceException, andSupportsNamespaces: a catalog that is not cannot be asked about, or made to create, the namespace of the table, so it fails with a message naming the catalog whatever the lookup reported.Then, if the identifier has a namespace, it asks the catalog with
namespaceExistsand creates the namespace when it is missing, tolerating a concurrent creation (AlreadyExistsException). The lookup is not used for this because it cannot be: Glue reports the missing namespace asNoSuchTableException, andTableMetadataCache.existsreturns the exception only when it has just looked the table up, not when it answers from the cache, which is whatDynamicRecordProcessorhands to the updater (it asks the cache first). The old check only ran in the first case, so in the processor flow the namespace was not created even for a catalog that throwsNoSuchNamespaceException.The change and its tests are identical in the three Flink version directories:
v1.20,v2.2,v2.3.Testing
TestTableUpdater#testTableCreationWhenCatalogReportsMissingNamespaceAsNoSuchTable: anInMemoryCatalogwhoseloadTablethrowsNoSuchTableExceptionfor a missing namespace, as Glue does. It fails without the change and passes with it.TestTableUpdater#testNamespaceIsCreatedWhenTheCacheAlreadyHoldsTheMissingTable: the flow ofDynamicRecordProcessor: the cache is asked first, so the updater gets the cached answer with no exception, for a catalog that reportsNoSuchNamespaceException. The namespace was not created before this change in that flow, and it is now.TestTableUpdater#testMissingNamespaceOfCatalogWithoutNamespaceSupportFailsand#testCatalogWithoutNamespaceSupportFailsWhateverTheLookupReported: a catalog withoutSupportsNamespacesfails with the message above, whether the lookup reportedNoSuchNamespaceException,NoSuchTableException, or the answer came from the cache.TestTableUpdater#testUnexpectedFailureOfTheTableLookupIsNotSwallowed: an unexpected exception in the lookup result fails the check.org.apache.iceberg.flink.sink.dynamicpass in the three versions (297, 317, 320 tests);spotlessApplyis clean.