github-actions[bot] commented on code in PR #66269:
URL: https://github.com/apache/doris/pull/66269#discussion_r3701068615
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogFactory.java:
##########
@@ -170,11 +170,20 @@ private static CatalogIf createCatalog(long catalogId,
String name, String resou
}
}
- // set some default properties if missing when creating catalog.
- // both replaying the creating logic will call this method.
- catalog.setDefaultPropsIfMissing(isReplay);
+ return finishCatalogCreation(catalog, isReplay);
+ }
+
+ static ExternalCatalog finishCatalogCreation(ExternalCatalog catalog,
boolean isReplay) throws DdlException {
+ // Set some default properties if missing when creating catalog.
+ // Both replaying the creating logic will call this method.
+ if (isReplay) {
+ catalog.setDefaultPropsIfMissing(true);
+ return catalog;
+ }
- if (!isReplay) {
+ boolean creationFinished = false;
+ try {
+ catalog.setDefaultPropsIfMissing(false);
catalog.checkWhenCreating();
// This will check if the customized access controller can be
created successfully.
// If failed, it will throw exception and the catalog will not be
created.
Review Comment:
**[P1] Do not abandon a live controller during dry-run validation**
`initAccessController(true)` still invokes the controller factory and simply
drops the returned instance. That is not a side-effect-free validation for
Ranger: `RangerHiveAccessController` schedules an audit flusher on a static
single-thread executor, and that task loops forever. The first dry-run instance
therefore retains its discarded audit handler and occupies the only worker;
tasks for the later registered controller stay queued, and this new `onClose()`
cannot clean the object because nothing retained it. Please make dry-run
validation non-allocating or add a close contract and close the temporary
controller, with a test that successful validation leaves no discarded
scheduled task and the live controller's flusher can run.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogFactory.java:
##########
@@ -184,9 +193,12 @@ private static CatalogIf createCatalog(long catalogId,
String name, String resou
LOG.warn("Failed to init access controller", e);
throw new DdlException("Failed to init access controller: " +
e.getMessage());
}
+ creationFinished = true;
+ return catalog;
+ } finally {
Review Comment:
**[P2] Preserve the creation error when cleanup also fails**
This `finally` dispatches subclass `onClose()` while the catalog-creation
exception is pending, but those cleanup methods are not guaranteed to be
non-throwing for a partially initialized catalog. For example, an Iceberg REST
catalog with `test_connection=false`, an invalid `access_controller.class`, and
an invalid `iceberg.rest.security.type` first produces the access-controller
`DdlException`; `IcebergRestExternalCatalog.onClose()` then lazily calls
`getMetastoreProperties()`, whose security validation throws
`IllegalArgumentException`, so Java replaces the original creation error.
Please make partial cleanup non-throwing, or catch cleanup failures and
attach/log them while preserving the primary exception, and cover a real
Iceberg REST failure rather than only a mocked `onClose()`.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/CatalogFactory.java:
##########
@@ -184,9 +193,12 @@ private static CatalogIf createCatalog(long catalogId,
String name, String resou
LOG.warn("Failed to init access controller", e);
throw new DdlException("Failed to init access controller: " +
e.getMessage());
}
+ creationFinished = true;
+ return catalog;
+ } finally {
+ if (!creationFinished) {
+ catalog.onClose();
Review Comment:
**[P1] Keep failed-candidate cleanup from removing the winner's controller**
This calls registered-catalog teardown for an object that was never
registered. `CatalogMgr.createCatalog()` deliberately releases its read lock
before construction, so two same-name creates can pass the precheck; if one
registers while the other later fails validation, the loser reaches this
`onClose()`. `ExternalCatalog.onClose()` removes the global access-controller
entry solely by name even though this candidate only performed
`initAccessController(true)` and never owned that entry. If the removal lands
after the winner's lazy loader installs its custom controller but before the
loader's post-check, it stores `defaultAccessController` instead and subsequent
checks keep using the default. Please use a resource-only/identity-aware
cleanup for unregistered candidates and add a latch-based same-name creation
test that preserves the winner's controller.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
