github-actions[bot] commented on code in PR #67978:
URL: https://github.com/apache/doris/pull/67978#discussion_r4215018762
##########
regression-test/suites/external_table_p0/lance/test_lance_index_admission.groovy:
##########
@@ -265,7 +316,9 @@ suite("test_lance_index_admission",
"p0,external,nonConcurrent") {
Review Comment:
[P3] Clean up the per-run Lance catalogs after stub rejection. This suite
unpauses dispatch and waits for NOT_COMMITTED, but the stub's clean
NOT_IMPLEMENTED rejection has `externalMetadataAdvanced=false`, so
classification is NOT_COMMITTED/NOT_REQUIRED and the job no longer holds a
fence or quota; the comment that refresh is still owed is stale. The
quota-table job needs the same convergence check before dropping this catalog.
`test_lance_index_dispatch.groovy` also restores pause to false and leaves its
own unique catalog. Repeated runs on a shared cluster therefore accumulate
catalogs indefinitely; wait for all admitted jobs to resolve, then drop each
catalog on success.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceExternalCatalog.java:
##########
@@ -101,10 +105,74 @@ public String resolveCurrentIndexJobLocator(String
dbName, String tableName) {
try {
return withClient(current ->
current.resolveCurrentIndexJobLocator(dbName, tableName));
} catch (Exception e) {
+ LOG.warn("failed to resolve the current dataset locator of {}.{}
in lance catalog {}",
+ dbName, tableName, getName(), e);
return null;
}
}
+ /**
+ * Three-valued resolution of the dataset the given names point at, for
callers
+ * that take a durable action on the verdict and must not fold "verified
gone"
+ * and "could not tell" together. {@link #resolveCurrentIndexJobLocator}
returns
+ * null for both on purpose (SHOW's fail-closed rule folds them); this form
+ * distinguishes them.
+ *
+ * <p>Callers pass their LOCAL spelling of the names — no remote-cased
names are
+ * required or assumed. Admission persists local database and table names,
which
+ * can be lowercase while the namespace stores {@code Foo.Bar}, and the
local
+ * lookup itself can miss transiently, so a case-sensitive resolve of the
local
+ * spelling could false-prove absence ({@code Foo.Bar} would read as gone
and a
+ * durable caller would act on it). Absence is therefore proven only
through
+ * full case-insensitive listings: {@code VERIFIED_ABSENT} requires the
whole
+ * database listing to miss the database, or the uniquely matched remote
database's table
+ * listing to miss the table. A name found in a listing is resolved under
its
+ * REMOTE spelling to obtain the durable locator; any listing or resolve
failure
+ * — provider unreachable, credentials expired, a namespace answer that
+ * contradicts its own listing — is logged (the sanitized client chain
already
+ * masks locators and credentials) and reported as
+ * {@link LanceIndexDatasetCheck.Outcome#UNRESOLVED}, so an outage can
never be
+ * read as "dataset gone". Multiple case-equivalent names are ambiguous and
+ * return UNRESOLVED, regardless of listing order or an exact-case match.
+ *
+ * <p>The listings are paid only on the local-resolution-miss path: callers
+ * reach this check after their own local db/table lookup already came back
+ * empty, so the namespace round-trips buy the absence proof instead of
adding
+ * overhead to the resolvable common path.
+ */
+ public LanceIndexDatasetCheck checkIndexJobDataset(String dbName, String
tableName) {
Review Comment:
[P2] Bound a single absence-proof refresh attempt. This new call can walk
every nested namespace and every `listNamespaces` page before it even reaches
the table listing; a large or slow namespace makes one refresh take many
provider RPC timeouts. The dispatcher subtracts its refresh budget only after
`driveOneRefresh` returns, so its between-job cap does not protect
deadline/epoch sweeps or PENDING dispatch from this one attempt. This is
distinct from the earlier multi-job delay: run the proof away from the sole
sweep thread or make its work resumable/time bounded while keeping the refresh
obligation unresolved.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceExternalCatalog.java:
##########
@@ -101,10 +105,74 @@ public String resolveCurrentIndexJobLocator(String
dbName, String tableName) {
try {
return withClient(current ->
current.resolveCurrentIndexJobLocator(dbName, tableName));
} catch (Exception e) {
+ LOG.warn("failed to resolve the current dataset locator of {}.{}
in lance catalog {}",
+ dbName, tableName, getName(), e);
return null;
}
}
+ /**
+ * Three-valued resolution of the dataset the given names point at, for
callers
+ * that take a durable action on the verdict and must not fold "verified
gone"
+ * and "could not tell" together. {@link #resolveCurrentIndexJobLocator}
returns
+ * null for both on purpose (SHOW's fail-closed rule folds them); this form
+ * distinguishes them.
+ *
+ * <p>Callers pass their LOCAL spelling of the names — no remote-cased
names are
+ * required or assumed. Admission persists local database and table names,
which
+ * can be lowercase while the namespace stores {@code Foo.Bar}, and the
local
+ * lookup itself can miss transiently, so a case-sensitive resolve of the
local
+ * spelling could false-prove absence ({@code Foo.Bar} would read as gone
and a
+ * durable caller would act on it). Absence is therefore proven only
through
+ * full case-insensitive listings: {@code VERIFIED_ABSENT} requires the
whole
+ * database listing to miss the database, or the uniquely matched remote
database's table
+ * listing to miss the table. A name found in a listing is resolved under
its
+ * REMOTE spelling to obtain the durable locator; any listing or resolve
failure
+ * — provider unreachable, credentials expired, a namespace answer that
+ * contradicts its own listing — is logged (the sanitized client chain
already
+ * masks locators and credentials) and reported as
+ * {@link LanceIndexDatasetCheck.Outcome#UNRESOLVED}, so an outage can
never be
+ * read as "dataset gone". Multiple case-equivalent names are ambiguous and
+ * return UNRESOLVED, regardless of listing order or an exact-case match.
+ *
+ * <p>The listings are paid only on the local-resolution-miss path: callers
+ * reach this check after their own local db/table lookup already came back
+ * empty, so the namespace round-trips buy the absence proof instead of
adding
+ * overhead to the resolvable common path.
+ */
+ public LanceIndexDatasetCheck checkIndexJobDataset(String dbName, String
tableName) {
+ try {
+ String remoteDbName = findUniqueIgnoreCase(withClient(current ->
current.listDatabaseNames()), dbName);
+ if (remoteDbName == null) {
+ return LanceIndexDatasetCheck.verifiedAbsent();
+ }
+ String remoteTableName = findUniqueIgnoreCase(
+ withClient(current ->
current.listTableNames(remoteDbName)), tableName);
+ if (remoteTableName == null) {
+ return LanceIndexDatasetCheck.verifiedAbsent();
+ }
+ return LanceIndexDatasetCheck.present(
+ withClient(current ->
current.resolveCurrentIndexJobLocator(remoteDbName, remoteTableName)));
Review Comment:
[P1] Sanitize provider failures before logging them. On a local lookup miss,
this catch logs the original provider exception and stack, while `withClient`
and the list/describe methods do not sanitize it. The existing admission test
injects a `describeTable` failure containing an access key, secret key, and
credential-bearing URI; this path would write those values to FE logs. The new
`resolveCurrentIndexJobLocator` warning and the dispatcher's metadata-refresh
catches also log raw provider errors. Log only bounded sanitized messages
without raw cause chains at these new sinks.
--
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]
