github-actions[bot] commented on code in PR #69252:
URL: https://github.com/apache/doris/pull/69252#discussion_r4310189777
##########
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:
[P2] Restore the thread-local context when MV setup fails
With materialized-view rewrite enabled, select `a.db` in a Remote Doris
catalog while healthy, then reset that catalog while its remote cluster is
unavailable and query a healthy internal table with a visible sync MV/rollup,
e.g. `SELECT k, SUM(v) FROM internal.db.t GROUP BY k`.
`MTMVPlanUtil.createBasicMvContext()` installs a fresh context as the
thread-local before copying the parent catalog/database via `setDatabase()`.
The Remote Doris health check now propagates its initialization failure through
this lookup, exiting before `MTMVCache.from()`, whose `finally` normally
restores the original context. `createSyncMvContexts()` catches and logs the
error, then `getMaterializationContextByHint()` dereferences the leaked
context's null `StatementContext` and throws an NPE, failing the unrelated
internal query. Previously the null fallback allowed setup to reach the
restoration. Guard the whole context construction with restoration, or copy
namespace state without metadata lookup, an
d add a failing-default-catalog/sync-MV regression.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/ConnectContext.java:
##########
@@ -1008,6 +1008,12 @@ public void setDatabase(String db) {
currentDbId = dbInstance.map(DatabaseIf::getId).orElse(-1L);
Review Comment:
[P2] Publish the selected database only after lookup succeeds
For a same-catalog `USE db_new` with `db_old` selected, `Env.changeDb()`
first validates `getDbOrDdlException(db_new)` and then calls `setDatabase()`,
which performs another lookup. A concurrent catalog refresh can invalidate the
mode-0 caches between those calls; if the metastore is then unavailable, that
second lookup now throws. `setDatabase()` has already assigned `currentDb =
db_new` but has not updated `currentDbId`, so USE returns ERR while the session
points at `db_new` with `db_old`'s ID. No caller restores the old selection.
Previously the null fallback let the setter finish with ID -1 and USE returned
OK. Resolve the requested database before assigning either session field, and
cover a refresh/failure between validation and assignment so a failed USE
preserves a consistent prior namespace.
##########
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:
[P2] Make external-view namespace switches exception safe
After a session selects `a.db`, reset its mode-0 cache and make its
metastore unavailable, then query a healthy external view with `SELECT * FROM
b.db.v`. `BindRelation.parseAndAnalyzeExternalView()` analyzes the view under
`b`, but its `finally` calls `ctx.setDatabase(previousDb)`, which now
propagates this lookup failure from unrelated catalog `a` and rejects the valid
query. Previously the failed lookup returned null and restoration completed.
This is distinct from the fixed forwarded-statement restoration.
The same method also calls `setDatabase(externalDb)` before entering its
`try`: a concurrent refresh of `b` followed by lookup failure there now skips
restoration and leaves the session switched to `b` after the failed SELECT.
Protect the entire namespace switch with cleanup and restore saved namespace
state without reloading remote metadata. Cover both failed entry and
restoration in cross-catalog view tests.
--
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]
