SameerMesiah97 commented on code in PR #69107:
URL: https://github.com/apache/airflow/pull/69107#discussion_r3488614428
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py:
##########
@@ -457,10 +458,27 @@ def filter_authorized_dag_ids(
cache_key = (user.get_id(), method, team_name, frozenset(dag_ids))
def query_keycloak() -> set[str]:
- kwargs: dict = dict(dag_ids=dag_ids, user=user, method=method)
- if team_name is not None:
- kwargs["team_name"] = team_name
- return super(KeycloakAuthManager,
self).filter_authorized_dag_ids(**kwargs)
+ if not dag_ids:
+ return set()
+
+ max_workers = min(
+ len(dag_ids), conf.getint(CONF_SECTION_NAME,
CONF_REQUESTS_POOL_SIZE_KEY, fallback=10)
+ )
+
+ def check(dag_id: str) -> tuple[str, bool]:
+ details_kwargs: dict[str, Any] = {"id": dag_id}
+ if team_name is not None:
+ details_kwargs["team_name"] = team_name
+ return dag_id, self.is_authorized_dag(
Review Comment:
Have you looked at whether `is_authorized_dag() `or the underlying HTTP
client becomes the limiting factor here? It would be good to understand whether
increasing the thread count beyond the HTTP connection pool size actually
improves throughput.
##########
providers/keycloak/src/airflow/providers/keycloak/auth_manager/keycloak_auth_manager.py:
##########
@@ -457,10 +458,27 @@ def filter_authorized_dag_ids(
cache_key = (user.get_id(), method, team_name, frozenset(dag_ids))
def query_keycloak() -> set[str]:
- kwargs: dict = dict(dag_ids=dag_ids, user=user, method=method)
- if team_name is not None:
- kwargs["team_name"] = team_name
- return super(KeycloakAuthManager,
self).filter_authorized_dag_ids(**kwargs)
+ if not dag_ids:
+ return set()
+
+ max_workers = min(
+ len(dag_ids), conf.getint(CONF_SECTION_NAME,
CONF_REQUESTS_POOL_SIZE_KEY, fallback=10)
Review Comment:
Using the number of DAGs as the upper bound for the number of threads makes
sense. But could you clarify the rationale behind using
CONF_REQUESTS_POOL_SIZE_KEY? Correct me if I am wrong but I believe you are
using it because it is the limit for the size of the HTTP connection pool for
Keycloak requests. It would not hurt to add a comment here to explain why you
are using these 2 variables to determine `max_workers`.
--
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]
