raghav-reglobe commented on code in PR #64966:
URL: https://github.com/apache/doris/pull/64966#discussion_r3553840082
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java:
##########
@@ -1290,6 +1291,57 @@ public List<String> listViewNames(String db) {
}
}
+ /**
+ * Wraps an idempotent read op with re-auth-on-401 self-heal. The Iceberg
REST
+ * OAuth client caches its Polaris token; if that token expires or is
invalidated
+ * (e.g. a token-refresh POST failed under Polaris load), the client
wedges and
+ * every call returns NotAuthorizedException (401) -- it does NOT re-auth
on its
+ * own, so the catalog stays unusable until the FE is restarted. Detect
that, force
+ * a full catalog-client rebuild on the parent catalog
+ * (ExternalCatalog.resetToUninitialized(true) -> fresh RESTSessionCatalog
+ fresh
+ * OAuth token, and the stale "db not found" cache entries are
invalidated), adopt
+ * the fresh client, and retry the op ONCE. Bounded to a single retry;
concurrent
+ * 401s coalesce (only the thread still holding the wedged client
rebuilds, the rest
+ * adopt the shared rebuilt client). Idempotent reads only -- never
in-flight
+ * mutations/commits.
+ */
+ private <T> T executeWithReauthRetry(Callable<T> op) throws Exception {
+ Catalog attemptedOn = this.catalog;
+ try {
+ return executionAuthenticator.execute(op);
+ } catch (Exception e) {
+ if (!isAuthExpired(e)) {
+ throw e;
+ }
+ synchronized (dorisCatalog) {
+ if (this.catalog == attemptedOn) {
+ LOG.warn("catalog {}: Polaris returned 401/Not-authorized
(cached OAuth token "
+ + "is stale) -- rebuilding the catalog client +
re-authing, then retrying once.",
+ dorisCatalog.getName());
+ dorisCatalog.resetToUninitialized(true);
+ dorisCatalog.makeSureInitialized();
+ ExternalMetadataOps rebuilt =
dorisCatalog.getMetadataOps();
+ if (rebuilt instanceof IcebergMetadataOps
+ && ((IcebergMetadataOps) rebuilt).getCatalog() !=
null) {
+ this.catalog = ((IcebergMetadataOps)
rebuilt).getCatalog();
+ this.nsCatalog = (SupportsNamespaces) this.catalog;
+ this.executionAuthenticator =
dorisCatalog.getExecutionAuthenticator();
+ }
+ }
+ // else: another thread already rebuilt the shared client ->
just retry on it.
+ }
+ return executionAuthenticator.execute(op);
+ }
+ }
+
+ private static boolean isAuthExpired(Throwable e) {
+ Throwable root = ExceptionUtils.getRootCause(e);
+ Throwable t = (root != null) ? root : e;
+ return t instanceof NotAuthorizedException
+ ||
"NotAuthorizedException".equals(t.getClass().getSimpleName())
+ || (t.getMessage() != null && t.getMessage().contains("Not
authorized"));
Review Comment:
Thanks — both points taken, and I've restructured the PR around your
suggestion.
Here's the sanitized catalog statement (the REST server is Apache Polaris;
the wedge should reproduce with any oauth2 client-credential REST catalog whose
auth server is briefly unreachable at token-refresh time):
```sql
CREATE CATALOG my_iceberg PROPERTIES (
'type' = 'iceberg',
'iceberg.catalog.type' = 'rest',
'iceberg.rest.uri' = 'http://polaris.example.internal:8181/api/catalog',
'warehouse' = 'my_warehouse',
'iceberg.rest.security.type' = 'oauth2',
'iceberg.rest.oauth2.credential' = '<client_id>:<client_secret>',
'iceberg.rest.oauth2.server-uri' =
'http://polaris.example.internal:8181/api/catalog/v1/oauth/tokens',
'iceberg.rest.oauth2.scope' = 'PRINCIPAL_ROLE:ALL',
'iceberg.rest.vended-credentials-enabled' = 'true',
's3.region' = 'ap-south-1',
'metadata_refresh_interval_sec' = '28800',
'iceberg.manifest.cache.enable' = 'true',
'iceberg.manifest.cache.capacity-mb' = '256',
'iceberg.manifest.cache.ttl-second' = '172800'
);
```
On the design point: agreed, and the recovery has moved out of
`IcebergMetadataOps` entirely. Since the OIDC change (#63068) Doris builds a
`RESTSessionCatalog` directly rather than a `RESTCatalog`, so the override
point I used is the session catalog: a new `ReauthenticatingRestSessionCatalog
extends BaseViewSessionCatalog`, constructed only by `IcebergRestProperties`
around the `RESTSessionCatalog` it already owns. Because `asCatalog()` /
`asViewCatalog()` are thin views that call back into the session catalog, the
default catalog, the view catalog, and per-user delegated sessions all inherit
the recovery with zero call-site changes — and non-REST catalogs never see any
of this logic.
The fragile detection is gone too: it's now typed-only
(`NotAuthorizedException` anywhere in the cause chain), no message-string or
class-name matching.
One deliberate exclusion: requests carrying a per-user delegated credential
are not recovered — a 401 there means that user's token is invalid, and
rebuilding the shared client can't fix that (and must not be triggered by it).
--
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]
