github-actions[bot] commented on code in PR #68305:
URL: https://github.com/apache/doris/pull/68305#discussion_r4059141399
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceNamespaceClient.java:
##########
@@ -178,34 +200,112 @@ boolean tableExists(String dbName, String tblName) {
}
LanceTableAccess resolveTableAccess(String dbName, String tableName) {
- DescribeTableResponse table = describeTable(dbName, tableName);
+ List<String> tableId = tableAccessKey(dbName, tableName);
+ if (tableAccessTtlNanos == 0) {
+ return loadTableAccess(tableId).access;
+ }
+ // Cache hits avoid the catalog-wide namespace lock as well as
filesystem or REST I/O.
+ return tableAccessCache.get(tableId, this::loadTableAccess).access;
+ }
+
+ LanceTableAccess resolveTableAccessUncached(String dbName, String
tableName) {
+ return loadTableAccess(tableAccessKey(dbName, tableName)).access;
+ }
+
+ void invalidateTableAccessCache() {
+ // Swap generations: a describe already in flight may finish for its
caller, but must
+ // never repopulate the cache used by reads admitted after an explicit
refresh.
+ tableAccessCache = newTableAccessCache();
+ }
+
+ private List<String> tableAccessKey(String dbName, String tableName) {
+ try {
+ return Collections.unmodifiableList(buildTableId(dbName,
tableName));
+ } catch (DdlException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ private CachedTableAccess loadTableAccess(List<String> tableId) {
+ DescribeTableResponse table = describeTable(tableId);
if (Boolean.TRUE.equals(table.getManagedVersioning())) {
throw new UnsupportedOperationException(
"Lance managed versioning is not supported by the current
BE reader");
}
String datasetUri = StringUtils.firstNonBlank(table.getTableUri(),
table.getLocation());
if (datasetUri == null) {
- throw new RuntimeException("Lance namespace returned no table URI
for " + dbName + "." + tableName);
+ throw new RuntimeException("Lance namespace returned no table URI
for " + tableId);
}
// One option map serves both readers: the FE opens the dataset
through the Lance Java SDK
// and the BE through lance-c, so neither can end up with credentials
the other lacks. The
// dataset URL picks the option vocabulary, the same way Lance picks a
provider from it.
Map<String, String> storageOptions =
LanceStorageOptions.fromDorisAndVendedStorageOptions(datasetUri,
storageProperties, table.getStorageOptions());
- return new LanceTableAccess(datasetUri, storageOptions);
+ return new CachedTableAccess(new LanceTableAccess(datasetUri,
storageOptions),
+ tableAccessTtlNanos(table.getStorageOptions()));
}
- private DescribeTableResponse describeTable(String dbName, String
tableName) {
+ private long tableAccessTtlNanos(Map<String, String> vendedOptions) {
+ if (vendedOptions == null || vendedOptions.isEmpty()) {
Review Comment:
[P1] Treat signed table URIs as expiring credentials
This early return assumes that empty `storage_options` means the access is
credential-free, but the namespace URI is retained verbatim and can itself
carry presigned/SAS credentials in userinfo or query parameters (the existing
`LanceIndexDatasetLocator` explicitly recognizes those forms). Such a response
has no option-map `expires_at_millis`, so it receives the full configured cache
TTL and may be reused after the URI signature expires. Please classify the
complete access response: disable caching for credential-bearing URIs unless a
trustworthy deadline is available, or bind their TTL to that deadline, and
cover the signed-URI/no-options case.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalMetaCacheMgr.java:
##########
@@ -484,11 +486,13 @@ public void removeCatalogByEngine(long catalogId, String
engine) {
}
public void invalidateDb(long catalogId, String dbName) {
+ invalidateLanceTableAccess(catalogId);
routeCatalogEngines(catalogId, cache -> safeInvalidate(
cache, catalogId, "invalidateDb", () ->
cache.invalidateDb(catalogId, dbName)));
}
public void invalidateTable(long catalogId, String dbName, String
tableName) {
+ invalidateLanceTableAccess(catalogId);
Review Comment:
[P1] Invalidate replayed refreshes without requiring a cached table object
This hook is reached only after `replayRefreshTable` finds an
`ExternalTable`, but replay deliberately performs a cache-only lookup and
returns when that object was evicted. The table-object cache is bounded at
1,000 entries and has no removal listener, while this independent access cache
holds 10,000 entries, so a follower can retain the pre-refresh URI/options,
skip invalidation, then rebuild the table and hit that stale entry after the
primary's `REFRESH TABLE`. Please retire Lance access by catalog/log identity
before the replay early return (catalog-wide clearing is already intentional
for name mapping), and add a replay test with the table object absent but
access still cached.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalMetaCacheMgr.java:
##########
@@ -484,11 +486,13 @@ public void removeCatalogByEngine(long catalogId, String
engine) {
}
public void invalidateDb(long catalogId, String dbName) {
+ invalidateLanceTableAccess(catalogId);
Review Comment:
[P2] Do not flush access entries for routine DB-object eviction
`invalidateDb` is also called automatically when the catalog's local
database-object cache evicts or expires any entry: its removal listener invokes
`resetMetaToUninitialized`, which reaches this method without checking the
removal cause. This new call then rotates the entire 10,000-entry Lance access
cache. With the default 1,000 database-object limit, traversing a larger or
churn-heavy catalog makes each ordinary eviction discard unrelated hot
table-access entries and defeats the namespace-I/O reduction this PR adds.
Please separate semantic refresh invalidation from local object-cache cleanup,
or target the evicted remote namespace instead of clearing the catalog-wide
generation.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/lance/LanceNamespaceClient.java:
##########
@@ -178,34 +200,112 @@ boolean tableExists(String dbName, String tblName) {
}
LanceTableAccess resolveTableAccess(String dbName, String tableName) {
- DescribeTableResponse table = describeTable(dbName, tableName);
+ List<String> tableId = tableAccessKey(dbName, tableName);
+ if (tableAccessTtlNanos == 0) {
+ return loadTableAccess(tableId).access;
+ }
+ // Cache hits avoid the catalog-wide namespace lock as well as
filesystem or REST I/O.
+ return tableAccessCache.get(tableId, this::loadTableAccess).access;
+ }
+
+ LanceTableAccess resolveTableAccessUncached(String dbName, String
tableName) {
+ return loadTableAccess(tableAccessKey(dbName, tableName)).access;
+ }
+
+ void invalidateTableAccessCache() {
+ // Swap generations: a describe already in flight may finish for its
caller, but must
+ // never repopulate the cache used by reads admitted after an explicit
refresh.
+ tableAccessCache = newTableAccessCache();
+ }
+
+ private List<String> tableAccessKey(String dbName, String tableName) {
+ try {
+ return Collections.unmodifiableList(buildTableId(dbName,
tableName));
+ } catch (DdlException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ private CachedTableAccess loadTableAccess(List<String> tableId) {
+ DescribeTableResponse table = describeTable(tableId);
if (Boolean.TRUE.equals(table.getManagedVersioning())) {
throw new UnsupportedOperationException(
"Lance managed versioning is not supported by the current
BE reader");
}
String datasetUri = StringUtils.firstNonBlank(table.getTableUri(),
table.getLocation());
if (datasetUri == null) {
- throw new RuntimeException("Lance namespace returned no table URI
for " + dbName + "." + tableName);
+ throw new RuntimeException("Lance namespace returned no table URI
for " + tableId);
}
// One option map serves both readers: the FE opens the dataset
through the Lance Java SDK
// and the BE through lance-c, so neither can end up with credentials
the other lacks. The
// dataset URL picks the option vocabulary, the same way Lance picks a
provider from it.
Map<String, String> storageOptions =
LanceStorageOptions.fromDorisAndVendedStorageOptions(datasetUri,
storageProperties, table.getStorageOptions());
- return new LanceTableAccess(datasetUri, storageOptions);
+ return new CachedTableAccess(new LanceTableAccess(datasetUri,
storageOptions),
+ tableAccessTtlNanos(table.getStorageOptions()));
}
- private DescribeTableResponse describeTable(String dbName, String
tableName) {
+ private long tableAccessTtlNanos(Map<String, String> vendedOptions) {
+ if (vendedOptions == null || vendedOptions.isEmpty()) {
+ return tableAccessTtlNanos;
+ }
+ // Vended options can contain temporary credentials. Never assume they
are permanent
+ // when expiry is absent, and reserve time for planning and dispatch
to the BE.
+ String expiry = vendedOptions.get("expires_at_millis");
+ if (expiry == null) {
+ return 0;
+ }
try {
- List<String> tableId = buildTableId(dbName, tableName);
- DescribeTableRequest request = new
DescribeTableRequest().id(tableId).withTableUri(true)
- .vendCredentials(LANCE_REST.equals(catalogType));
- synchronized (namespaceLock) {
- return namespace.describeTable(request);
+ long expiresAtMillis = Long.parseLong(expiry);
+ long now = currentTimeMillis.getAsLong();
+ if (expiresAtMillis <= now) {
+ return 0;
}
- } catch (DdlException e) {
- throw new RuntimeException(e);
+ long remainingMillis = Math.max(0, expiresAtMillis - now -
TimeUnit.SECONDS.toMillis(30));
Review Comment:
[P1] Do not reuse credentials that may expire during the scan
The 30-second reserve only guarantees that these cached credentials survive
planning and dispatch, but the option map is sent unchanged to the BE and the
Lance scanner keeps using that same object-store client for every batch; there
is no credential refresh path. A cache hit near this deadline can therefore
launch a scan with only about 30 seconds remaining and fail mid-query, even
though a fresh `describeTable` at that time would vend renewed credentials.
Because query duration is unbounded, a fixed margin cannot make credential
caching safe. Please cache only the stable target while resolving vended
credentials per read, or add end-to-end credential renewal/lease semantics.
--
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]
