sadpandajoe commented on code in PR #45554:
URL: https://github.com/apache/superset/pull/45554#discussion_r4381025606
##########
superset/semantic_layers/exceptions.py:
##########
@@ -110,7 +151,12 @@ def execute_semantic_query(
with provider_completeness():
return dispatcher(*args, **kwargs)
except SemanticQueryRejectedError as ex:
- raise SemanticLayerQueryRejectedError(ex.code) from ex
+ raise SemanticLayerQueryRejectedError(ex.code) from None
+ except ProviderRequestError as ex:
Review Comment:
This `ProviderRequestError` mapping only wraps the dispatched query. A
provider that uses the supplied transport in `from_configuration()` or
`get_dimensions()` can raise `ProviderRequestError(HTTP_STATUS, status=401)`
earlier, e.g. via `_resolve_external_view_metadata` in the MCP
`get_table(view_id=...)` path, which isn't wrapped here and lands in the
generic fallback in `_finalized_get_table` as `InternalError` rather than the
`QueryError` the docs promise. Should the preparation/metadata calls go through
the same normalization?
##########
docs/developer_docs/extensions/semantic-provider-boundary.md:
##########
@@ -0,0 +1,96 @@
+---
+title: Semantic provider readiness and failures
+---
+
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+
+# Semantic provider readiness and failures
+
+## Schema discovery
+
+Both schema methods require keyword-only `transport` and `readiness`. Import
+`SemanticCredentialReadiness` from `superset_core.semantic_layers.config` (also
+exported from `superset_core.semantic_layers`). It is a plain Enum, not a
string.
+
+```python
+@classmethod
+def get_configuration_schema(
+ cls, configuration: MyConfig | None = None, *,
+ transport: ProviderTransport, readiness: SemanticCredentialReadiness,
+) -> dict[str, Any]:
+ schema: dict[str, Any] = MyConfig.model_json_schema()
+ if readiness is not SemanticCredentialReadiness.RESOLVED:
+ return schema
+ if configuration is None or not configuration.password:
+ return schema
+ # Discover only after provider-specific required-field checks.
+ # Use the supplied transport for any HTTP request.
+ return schema
+```
+
+Apply the same positive identity check in `get_runtime_schema(configuration,
+runtime_data=None, *, transport, readiness)`. Strings such as `"resolved"`,
booleans
+and unrelated objects must not enable discovery. A static schema must perform
no
+network activity or credential lookup and must not echo secrets in
defaults/options.
+Readiness is neither complete configuration nor authenticated/authorized
access.
+Providers still validate required fields and allowlisted runtime overrides.
+
+The host checks submitted preview values before partial validation can discard
an
+unresolved field. A preview does not fetch or restore saved secrets. Runtime
+schemas use the saved connection after the existing layer access check. The
host
+alone understands its masking convention; client payload fields never select
the
+trusted readiness keyword. Static masking and fallback calls explicitly pass
+UNRESOLVED. Direct SDK hosts must supply an explicit state using their own
secret
+resolution rules, without adopting Superset's mask.
+
+This is a coordinated schema API upgrade. Update all overrides and direct
callers
+with the host/core pin containing transport and readiness; there is no fallback
+that retries without safety arguments. The factory's
transport/context/credentials
Review Comment:
This says the factory contract is unchanged, but
`SemanticLayer.from_configuration` gains a required keyword-only `transport`
argument in this PR (and UPDATING.md says so). An extension author who follows
this paragraph and only updates the two schema hooks will hit a `TypeError` for
the unexpected `transport` keyword when the host constructs their layer. Should
this sentence instead call out the new `transport` parameter on the factory?
##########
superset/utils/network.py:
##########
@@ -260,3 +265,161 @@ def is_host_up(host: str) -> bool:
return False
return output == 0
+
+
+class ConnectionDeadline(Protocol):
+ """A caller-owned deadline; socket code does not choose request policy."""
+
+ def remaining(self) -> float: ...
+ def watch(self, sock: socket.socket) -> None: ...
+
+
+PROVIDER_DEADLINE: ContextVar[ConnectionDeadline | None] = ContextVar(
+ "provider_connection_deadline", default=None
+)
+_RESOLVER: ThreadPoolExecutor | None = None
+_RESOLVER_LOCK: Lock = Lock()
+
+
+class NonPublicAddressError(OSError):
+ """Resolution included an address refused by the shared destination
rules."""
+
+
+def _resolve_addresses(
+ host: str, port: int, timeout: float, *, public_only: bool
+) -> list[str]:
+ """Bound waiting and pin the complete answer set before any connection."""
+ global _RESOLVER # pylint: disable=global-statement
+ if timeout <= 0:
+ raise TimeoutError
+ with _RESOLVER_LOCK:
+ if _RESOLVER is None:
+ _RESOLVER = ThreadPoolExecutor(8, "provider-dns")
+ lookup: Future[Any] = _RESOLVER.submit(
+ socket.getaddrinfo, host, port, socket.AF_UNSPEC,
socket.SOCK_STREAM
+ )
+ try:
+ answers: Any = lookup.result(timeout=timeout)
+ finally:
+ lookup.cancel() # Queued work is disposable; running system DNS
cannot stop.
Review Comment:
Agreed—`lookup.cancel()` can't stop a `getaddrinfo` that is already running,
so eight stalled lookups keep all eight `provider-dns` workers busy and every
later request, even to a numeric IP, queues until its deadline expires. Is a
shared pool the intended trade-off here, or should stalled lookups be isolated
per request?
--
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]
