Gabriel39 commented on code in PR #68305:
URL: https://github.com/apache/doris/pull/68305#discussion_r4059237440
##########
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:
Fixed in 46d4b022f0. Removed access-cache invalidation from generic
invalidateDb; explicit database refresh/replay and namespace removal now
invalidate through semantic lifecycle paths. The regression warms access,
resets an unrelated database object, and verifies that the next read reuses the
entry. Explicit refresh and namespace-removal tests verify that those
operations still force resolution.
##########
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:
Fixed in 46d4b022f0. Refresh replay now invalidates Lance access by catalog
identity before looking up cached database/table objects. Coverage includes
absent tables, absent databases, both name-based and legacy ID-based table
logs, and database refresh replay with an absent database. Invalidation does
not initialize the catalog client.
##########
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:
Fixed in 46d4b022f0. Cache eligibility now checks the URI as well as
storage_options. URI userinfo (including registry-based authorities), query
parameters, fragments, opaque URIs, and unparsable locators bypass caching. The
regression covers signed S3/SAS URIs and URI credentials with no
storage_options.
##########
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:
Fixed in 46d4b022f0. Removed the fixed 30-second reserve and disabled
access-result caching whenever describeTable supplies nonempty storage_options,
even with a reported expiry. Each read requests fresh vended access instead of
launching a scan with credentials aged by this cache. Plain REST responses
remain cacheable. The regression verifies repeated resolution despite a future
expiry. Credential renewal during an ongoing scan is outside this change.
--
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]
