924060929 commented on code in PR #69252:
URL: https://github.com/apache/doris/pull/69252#discussion_r4320797777
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -779,17 +779,8 @@ public ExternalDatabase<? extends ExternalTable>
getDbNullable(String dbName) {
if (StringUtils.isEmpty(dbName)) {
return null;
}
- try {
- makeSureInitialized();
- } catch (Exception e) {
- String diagnosticTrace = initErrorStackTrace(e);
- if (diagnosticTrace != null) {
- LOG.warn("failed to get db {} in catalog {}: {}", dbName,
name, diagnosticTrace);
- } else {
- LOG.warn("failed to get db {} in catalog {}", dbName, name, e);
- }
- return null;
- }
+ // Initialization errors do not establish that the requested database
is absent.
+ makeSureInitialized();
Review Comment:
Rechecked at `c8acefa09ab1`: this still applies. `Env.changeCatalog()`
publishes the target catalog and clears the current database at lines 6619–6622
before resolving the remembered database. If that lookup fails after a catalog
reset, `SWITCH ext` returns an error but leaves the session on `ext` with no
selected database. The atomic assignments in `setDatabase()` do not restore the
previous catalog. Please restore the captured `NameSpaceContext` on failure (or
resolve before publishing), and add a failed remembered-database SWITCH test.
Static call-chain verification; no runtime reproduction in this review.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -779,17 +779,8 @@ public ExternalDatabase<? extends ExternalTable>
getDbNullable(String dbName) {
if (StringUtils.isEmpty(dbName)) {
return null;
}
- try {
- makeSureInitialized();
- } catch (Exception e) {
- String diagnosticTrace = initErrorStackTrace(e);
- if (diagnosticTrace != null) {
- LOG.warn("failed to get db {} in catalog {}: {}", dbName,
name, diagnosticTrace);
- } else {
- LOG.warn("failed to get db {} in catalog {}", dbName, name, e);
- }
- return null;
- }
+ // Initialization errors do not establish that the requested database
is absent.
+ makeSureInitialized();
Review Comment:
Rechecked at `c8acefa09ab1`: both HTTP callers still perform the optional
lookup before SQL analysis/submission (`StmtExecutionAction.java:114,149`).
Consequently, an unavailable default `ext.db` blocks a fully qualified query
against a healthy internal table, or a catalog-repair statement, before its
actual dependencies are examined. Please defer optional namespace-resolution
failures while retaining errors for actual database access, and cover both
endpoints. For the execution-endpoint control, also provide the healthy
internal database selected by the JDBC worker. Static call-chain verification;
no runtime reproduction in this review.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -1098,42 +1089,14 @@ protected ExternalDatabase<? extends ExternalTable>
buildDbForInit(String remote
// When running ut, disable this check to make ut pass.
// Because in ut, the database is not created in remote system.
if (checkExists && (!FeConstants.runningUnitTest || this instanceof
TestExternalCatalog)) {
- try {
- final String lookupLocalDbName = localDbName;
- // Reuse the shared names lookup helper so existence checks
follow the same miss-refresh policy.
- Boolean exists =
resolveDatabaseNameFromSnapshot(lookupLocalDbName, false,
- namesValue ->
namesValue.containsLocalName(lookupLocalDbName) ? Boolean.TRUE : null);
- if (!Boolean.TRUE.equals(exists)) {
- LOG.warn("Database {} does not exist in the remote system.
Skipping initialization.",
- localDbName);
- return null;
- }
- } catch (RuntimeException e) {
- // Handle "Found conflicting" exception explicitly
- if (e.getMessage().contains(FOUND_CONFLICTING)) {
- LOG.error(e.getMessage());
- throw e; // Rethrow to let the caller handle this critical
issue
- } else {
- // Any errors other than name conflicts, we default to not
finding the database
- String diagnosticTrace = initErrorStackTrace(e);
- if (diagnosticTrace != null) {
- LOG.warn("Failed to check db {} exist in remote
system, ignore it: {}",
- localDbName, diagnosticTrace);
- } else {
- LOG.warn("Failed to check db {} exist in remote
system, ignore it.", localDbName, e);
- }
- return null;
- }
- } catch (Exception e) {
- // If connection failed, it will throw exception.
- // ignore it and treat it as not exist.
- String diagnosticTrace = initErrorStackTrace(e);
- if (diagnosticTrace != null) {
- LOG.warn("Failed to check db {} exist in remote system,
ignore it: {}",
- localDbName, diagnosticTrace);
- } else {
- LOG.warn("Failed to check db {} exist in remote system,
ignore it.", localDbName, e);
- }
+ // Authentication and connection failures must propagate; only a
successful lookup can prove absence.
+ final String lookupLocalDbName = localDbName;
+ // Reuse the shared names lookup helper so existence checks follow
the same miss-refresh policy.
+ Boolean exists =
resolveDatabaseNameFromSnapshot(lookupLocalDbName, false,
Review Comment:
The initial lookup-failure case is fixed at `c8acefa09ab1`, but a later
lookup can still abort the same repair commands. After `getDbNullable()`
succeeds at `ConnectProcessor.java:595`, a concurrent catalog reset followed by
an outage can make `Env.changeDb()` at line 604 throw during either of its
repeated lookups (`Env.java:6643–6644`). That call is outside the new catch, so
`proxyExecute()` returns an error before executing the forwarded REFRESH/ALTER.
Please reuse the resolved database when publishing the optional namespace, or
defer failures across the complete optional restoration while preserving
privilege checks. Add deterministic failure injection after the first
successful lookup. Static call-chain verification; no runtime reproduction in
this review.
--
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]
